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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
