[PATCH] D51575: [clang-tidy] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore added inline comments.


================
Comment at: clang-tidy/google/FunctionNamingCheck.cpp:57
+      functionDecl(
+          isDefinition(),
+          unless(anyOf(isMain(), matchesName(validFunctionNameRegex(true)),
----------------
hokein wrote:
> stephanemoore wrote:
> > hokein wrote:
> > > any reason why we restrict to definitions only? I think we can consider declarations too.
> > 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.
> 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.
> 
> 
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.


================
Comment at: clang-tidy/google/FunctionNamingCheck.cpp:35
+  // non-standard capitalized character sequences including acronyms,
+  // initialisms, and prefixes of symbols (e.g., UIColorFromNSString). For this
+  // reason, the regex only verifies that the function name after the prefix
----------------
benhamilton wrote:
> benhamilton wrote:
> > 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.
> (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.)
> 
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.


================
Comment at: clang-tidy/google/FunctionNamingCheck.cpp:50
+
+void FunctionNamingCheck::registerMatchers(MatchFinder *Finder) {
+  // This check should only be applied to Objective-C sources.
----------------
benhamilton wrote:
> Wizard wrote:
> > 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).
> +1, I think the two checks should be substantially similar.
Implemented a fixit hint for functions of static storage class.


================
Comment at: clang-tidy/google/FunctionNamingCheck.h:21
+
+/// Finds function names that do not conform to the recommendations of the
+/// Google Objective-C Style Guide. Function names should be in upper camel case
----------------
benhamilton wrote:
> Worth mentioning this does not apply to Objective-C method names, nor Objective-C properties.
> 
Added a note that this check does not apply to Objective-C methods or properties.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51575