[PATCH] D55482: [clang-tidy] Improve google-objc-function-naming diagnostics 📙

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore marked 4 inline comments as done.
stephanemoore added inline comments.


================
Comment at: clang-tidy/google/FunctionNamingCheck.cpp:114-118
   diag(MatchedDecl->getLocation(),
-       "function name %0 not using function naming conventions described by "
-       "Google Objective-C style guide")
-      << MatchedDecl << generateFixItHint(MatchedDecl);
+       "%select{static|global}1 function name %0 must %select{be in|have an "
+       "appropriate prefix followed by}1 Pascal case as required by Google "
+       "Objective-C style guide")
+      << MatchedDecl << IsGlobal << generateFixItHint(MatchedDecl);
----------------
benhamilton wrote:
> Should we suggest making the function static when it fails this check? (I assume the vast majority of functions which fail this check should really be static.)
Are you asking whether we should suggest making global functions static when they are missing appropriate prefixes? If so, I think that would lead to undesired changes in various cases.

One common cause of this style violation is when someone endeavors to take a static function that is used in one translation unit and share it with other translation units by migrating it to a header. The Easy™ thing to do is migrate that static function to a header and convert it directly to a global function, avoiding the overhead of refactoring the function name and all call sites. The effort of the described refactoring approach is low compared to renaming the function to avoid symbol collisions in the global namespace as is generally recommended.

If a function's definition and declaration (if it exists) are both in an implementation file we can probably recommend making the function static though. Filed https://bugs.llvm.org/show_bug.cgi?id=39945.


================
Comment at: clang-tidy/google/FunctionNamingCheck.cpp:115
   diag(MatchedDecl->getLocation(),
-       "function name %0 not using function naming conventions described by "
-       "Google Objective-C style guide")
-      << MatchedDecl << generateFixItHint(MatchedDecl);
+       "%select{static|global}1 function name %0 must %select{be in|have an "
+       "appropriate prefix followed by}1 Pascal case as required by Google "
----------------
benhamilton wrote:
> I know "global" is the correct name, but I feel like "non-static" might be clearer to folks dealing with these error messages.
> 
> WDYT?
> 
I'm wary of saying "non-static" because namespaced functions in Objective-C++ are not subject to the cited rules. The term "non-static" seems like it could be interpreted to extend to namespaced functions which could potentially mislead someone into adding prefixes to namespaced functions in Objective-C++.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55482/new/

https://reviews.llvm.org/D55482