This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜
ClosedPublic

Authored by stephanemoore on Sep 1 2018, 10:11 PM.

Details

Summary

§1 Description

This check finds function names in function declarations in Objective-C files that do not follow the naming pattern described in the Google Objective-C Style Guide. Function names should be in UpperCamelCase and functions that are not of static storage class should have an appropriate prefix as described in the Google Objective-C Style Guide. The function main is a notable exception. Function declarations in expansions in system headers are ignored.

Example conforming function definitions:

static bool IsPositive(int i) { return i > 0; }
static bool ABIsPositive(int i) { return i > 0; }
bool ABIsNegative(int i) { return i < 0; }

A fixit hint is generated for functions of static storage class but otherwise the check does not generate a fixit hint because an appropriate prefix for the function cannot be determined.

§2 Test Notes

  • Verified clang-tidy tests pass successfully.
  • Used check_clang_tidy.py to verify expected output of processing google-objc-function-naming.m

Diff Detail

Repository
rL LLVM

Event Timeline

stephanemoore created this revision.Sep 1 2018, 10:11 PM

Minor fixes:

  • Fixed header guard.
  • Removed unnecessary imports in header.
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
60 ↗(On Diff #163638)

Please use alphabetical order.

Fixed alphabetical ordering of clang-tidy improvements in release notes.

stephanemoore marked an inline comment as done.Sep 2 2018, 10:01 AM
stephanemoore added inline comments.
docs/ReleaseNotes.rst
60 ↗(On Diff #163638)

Good catch. Fixed.

Nice! looks mostly good to me.

clang-tidy/google/FunctionNamingCheck.cpp
57 ↗(On Diff #163647)

any reason why we restrict to definitions only? I think we can consider declarations too.

unittests/clang-tidy/GoogleModuleTest.cpp
109 ↗(On Diff #163647)

nit: we don't need unittest for the check here, as it is well covered in the littest.

stephanemoore marked an inline comment as done.

Updated with changes:

  • Removed unit tests as other tests have been indicated to provide adequate coverage.
  • Added a comment explaining why only function definitions are evaluated.
stephanemoore marked an inline comment as done.Sep 3 2018, 12:08 AM
stephanemoore added inline comments.
clang-tidy/google/FunctionNamingCheck.cpp
57 ↗(On Diff #163647)

I restricted the check to function definitions because function declarations are sometimes associated with functions outside the control of the author. I have personally observed unfortunate cases where functions declared in the iOS SDK had incorrect—or seemingly incorrect—availability attributes that caused fatal dyld assertions during application startup. The check currently intends to avoid flagging function declarations because of the rare circumstances where an inflexible function declaration without a corresponding function definition is required.

I have added a comment explaining why only function definitions are flagged.

I am still open to discussion though.

unittests/clang-tidy/GoogleModuleTest.cpp
109 ↗(On Diff #163647)

Removed the unit tests.

JonasToth removed a subscriber: JonasToth.
hokein added inline comments.Sep 4 2018, 12:27 PM
clang-tidy/google/FunctionNamingCheck.cpp
57 ↗(On Diff #163647)

Thanks for the explanations.

I have a concern about the heuristic using here, it seems fragile -- if there is an inline function defined in a base header, the check will still give a warning to it if the source file .m #includes this header; it also limits the scope of the check, I think this check is flagged mostly on file-local functions (e.g. static functions, functions defined in anonymous namespace).

Flagging on function declaration doesn't seem too problematic (we already have a similar check objc-property-declaration does the same thing) -- our internal review system shows check warnings on changed code, if user's code includes a common header which violates the check, warnings on the header will not be shown; for violations in iOS SDK, we can use some filtering matchers (isExpansionInSystemHeader maybe work) to ignore all functions from these files.

Wizard added inline comments.Sep 4 2018, 1:10 PM
clang-tidy/google/FunctionNamingCheck.cpp
50 ↗(On Diff #163656)

Can we do some simple check to see if some easy fix can be provided just like objc-property-declaration check?
Something like static bool isPositive to static bool IsPositive and static bool is_upper_camel to IsUpperCamel. Such check can help provide code fix for a lot of very common mistake at a low cost (i.e. if the naming pattern cannot be simply recognized, just provide no fix).

benhamilton requested changes to this revision.Sep 5 2018, 9:09 AM

Thanks for this! Let's consolidate this with the property name checker (either simplify the logic there and allow arBiTRAryCapSAnYWHere or apply the same registered acronym logic here).

clang-tidy/google/FunctionNamingCheck.cpp
35 ↗(On Diff #163656)

Any reason why this is different from the implementation in the property name checker? Either we should allow both of:

void ARBiTraRilyNameDFuncTioN();
// ...
@property (...) id arBItrArIlyNameD;

or we should require that acronyms in the middle of the name be registered/known acronyms for both properties and functions.

I believe this diff allows arbitrary capitalization for functions, but we disallowed that for property names, so I think we should be consistent.

50 ↗(On Diff #163656)

+1, I think the two checks should be substantially similar.

clang-tidy/google/FunctionNamingCheck.h
21 ↗(On Diff #163656)

Worth mentioning this does not apply to Objective-C method names, nor Objective-C properties.

This revision now requires changes to proceed.Sep 5 2018, 9:09 AM
benhamilton added inline comments.Sep 5 2018, 2:07 PM
clang-tidy/google/FunctionNamingCheck.cpp
35 ↗(On Diff #163656)

(And, just to be clear, I don't feel strongly which direction we go — it's certainly less work to allow arbitrarily-named functions and properties than to maintain the acronym list.)

stephanemoore marked 3 inline comments as done.
stephanemoore edited the summary of this revision. (Show Details)

Implemented the following suggested changes:
• Restricted matching to function declarations that are not in expansions in system headers.
• Extended the check to all function declarations rather than just function definitions.
• Implemented a fixit hint for functions of static storage class.

Implemented the following suggested changes:
• Added a note that FunctionNamingCheck does not apply to Objective-C method name or property declarations.

stephanemoore marked an inline comment as done.

Cleaned up comment about FunctionNamingCheck not applying to Objective-C method or property declarations.

stephanemoore added inline comments.Sep 8 2018, 1:03 AM
clang-tidy/google/FunctionNamingCheck.cpp
57 ↗(On Diff #163647)

Good idea to use isExpansionInSystemHeader. I wasn't aware of that particular matcher. I have incorporated that into the matching.

I am still wary about flagging function declarations but I think that false positives should generally be marginal and we can monitor for them. I have extended the check to also include function declarations.

35 ↗(On Diff #163656)

I have been meaning to send out a proposal to change the objc-property-declaration check to allow arbitrary capitalization but I ended up sending this out first. Let me prep my proposal for that change simultaneously.

50 ↗(On Diff #163656)

Implemented a fixit hint for functions of static storage class.

clang-tidy/google/FunctionNamingCheck.h
21 ↗(On Diff #163656)

Added a note that this check does not apply to Objective-C methods or properties.

alexfh removed a reviewer: alexfh.Sep 18 2018, 2:17 AM
stephanemoore marked an inline comment as done.Nov 1 2018, 7:03 PM

https://reviews.llvm.org/D51832 is in review to update the objc-property-declaration check to allow arbitrary acronyms and initialisms.

Updated diff after pulling and merging.

stephanemoore marked 8 inline comments as done.Nov 1 2018, 7:09 PM
stephanemoore retitled this revision from [clang-tidy] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜 to [clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜.
benhamilton added inline comments.Nov 7 2018, 12:20 PM
docs/clang-tidy/checks/google-objc-function-naming.rst
20 ↗(On Diff #172296)

This is not actually handled by the check, right? (You even have a test which confirms this.)

As far as I can tell, this check essentially looks for at least one capital letter and no _ in function names, right?

stephanemoore marked an inline comment as done.

Added a test case for an extern function without an appropriate prefix (IsPrime).

stephanemoore added inline comments.Nov 8 2018, 4:07 PM
docs/clang-tidy/checks/google-objc-function-naming.rst
20 ↗(On Diff #172296)

It does flag this declaration because it's missing an appropriate prefix.

It looks like I neglected to add a test case for that. I have done so now.

aaron.ballman added inline comments.Nov 9 2018, 1:03 PM
clang-tidy/google/FunctionNamingCheck.cpp
55 ↗(On Diff #173244)

Elide braces.

59–60 ↗(On Diff #173244)

Do not use auto as the type is not mentioned in the initialization.

66 ↗(On Diff #173244)

Please make this the while loop condition rather than an explicit break.

86 ↗(On Diff #173244)

Elide braces.

98 ↗(On Diff #173244)

Elide braces. Also, didn't we get rid of the distinction between ObjcC1 and ObjC2?

116 ↗(On Diff #173244)

No need for this assertion. The check() function cannot be called without this being nonnull.

119 ↗(On Diff #173244)

I don't think this assert adds value.

122–124 ↗(On Diff #173244)

You can drop the explicit quotes around %0 and instead pass in MatchedDecl rather than MatchedDecl->getName().

stephanemoore marked 9 inline comments as done.

Updated with the following changes:
• Elided conditional braces to comply with LLVM style.
• Converted conditional loop break to loop condition in generateFixItHint.
• Fixed Objective-C language option check.
• Removed unnecessary assertions.
• Use MatchedDecl directly for formatting the diagnostic message.

Generally LGTM, but I leave it to someone more well-versed in ObjC to sign off that this actually does everything those users would expect.

docs/clang-tidy/checks/google-objc-function-naming.rst
12 ↗(On Diff #173544)

"Upper camel case" is more commonly known as Pascal case, btw (http://wiki.c2.com/?PascalCase).

benhamilton accepted this revision.Nov 12 2018, 8:38 AM
This revision is now accepted and ready to land.Nov 12 2018, 8:38 AM

Reworded "upper camel case" to "Pascal case".

stephanemoore marked an inline comment as done.Nov 13 2018, 1:57 PM

Thanks for the review everyone!

Let me know if there are any further changes that you want me to make or any further action required on my part to land this 😁

Thanks for the review everyone!

Let me know if there are any further changes that you want me to make or any further action required on my part to land this 😁

It's good to go in -- do you have commit access, or would you like someone to land this for you?

I received commit access yesterday so I believe I should be able to land this myself (after I get myself set up and land a test commit).

stephanemoore added inline comments.Nov 16 2018, 3:42 PM
clang-tidy/google/FunctionNamingCheck.cpp
63 ↗(On Diff #173936)

I forgot to invert this condition when I refactored the early return into the loop condition 😓

Fixed loop condition in generateFixItHint.

stephanemoore marked an inline comment as done.Nov 16 2018, 3:43 PM
This revision was automatically updated to reflect the committed changes.