This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Make google-objc-function-naming ignore implicit functions 🙈
ClosedPublic

Authored by stephanemoore on Feb 11 2019, 8:31 PM.

Details

Event Timeline

stephanemoore created this revision.Feb 11 2019, 8:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 8:31 PM
stephanemoore marked an inline comment as done.Feb 11 2019, 8:43 PM
stephanemoore added inline comments.
clang-tools-extra/test/clang-tidy/google-objc-function-naming.m
18

Thus far I have been unsuccessful in using line markers to simulate this declaration being in a system header but I did discover precedence for using NOLINT to suppress diagnostics in some of the clang-tidy tests:
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/google-runtime-int-std.cpp#L11
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/google-runtime-int.cpp#L6

I think it should be reasonable to suppress the diagnostic here with a comment explaining why. Let me know if you don't think that's an appropriate solution and I can continue investigating for a potential solution using line markers.

aaron.ballman added inline comments.Feb 12 2019, 5:32 AM
clang-tools-extra/test/clang-tidy/google-objc-function-naming.m
18

Personally, I would recommend adding stdio.h to extra\test\clang-tidy\Inputs\Headers and adding a -isystem to this test's RUN line. You could also add #pragma clang system_header to the file to be really sure it's treated as a system header. This gives us a place to add more stdio.h declarations in the future as well.

Create a new header stdio.h under //test/clang-tidy/Inputs/Headers/ to contain declaration of
printf and import the new header to google-objc-function-naming.m.

stephanemoore marked 2 inline comments as done.

Add a comment to the new stdio.h.

stephanemoore added inline comments.Feb 12 2019, 5:11 PM
clang-tools-extra/test/clang-tidy/google-objc-function-naming.m
18

That sounds like a better idea. Thanks for the suggestion!

Things seem to work without needing #pragma clang system_header so I left it out. If you want me to include that, I can do so.

stephanemoore marked an inline comment as done.Feb 12 2019, 7:27 PM
stephanemoore added inline comments.
clang-tools-extra/test/clang-tidy/Inputs/Headers/stdio.h
2

Do I need the licensing preamble on this test input header?

2–10

I noticed that some of the other example headers don't have #ifdef guards at all. I decided to still include them—perhaps mostly for my own sanity. I figured that it wasn't necessary to use the full LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_INPUTS_HEADER_STDIO_H for this header that is mimicking a system header so I went with the shorter _STDIO_H_ which I think better represents what actual stdio.h headers would have. Let me know if you think something else makes more sense.

aaron.ballman accepted this revision.Feb 13 2019, 6:31 AM

LGTM!

clang-tools-extra/test/clang-tidy/Inputs/Headers/stdio.h
2

Yes, please add it.

2–10

I don't think it matters all that much for our tests; I'm fine with the way you have it currently.

This revision is now accepted and ready to land.Feb 13 2019, 6:31 AM

Added licensing preamble to stdio.h

stephanemoore marked 4 inline comments as done.Feb 19 2019, 8:43 PM
stephanemoore added inline comments.
clang-tools-extra/test/clang-tidy/Inputs/Headers/stdio.h
2

Added the licensing preamble. Let me know if anything looks amiss.

Still LGTM, thanks for adding the license snippet!

This revision was automatically updated to reflect the committed changes.
stephanemoore marked an inline comment as done.

Thanks for the review!