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

[stephanemoore]

Summary

The diagnostics from google-objc-function-naming check will be more
actionable if they provide a brief description of the requirements from
the Google Objective-C style guide. The more descriptive diagnostics may
help clarify that functions in the global namespace must have an
appropriate prefix followed by Pascal case (engineers working previously
with static functions might not immediately understand the different
requirements of static and non-static functions).

Test Notes:
Verified against the clang-tidy tests.

[MyDeveloperDay]

test/clang-tidy/google-objc-function-naming.m

8

I realize there are words that begin with 'is...' but you could detect if the function returned a boolean and started with "is,has,does" and could this extrapolate to IsPositive() HasSomething(), DoesHaveSomething(), this might generate a better fixit candidate? (just a suggestion feel free to ignore)

[aaron.ballman]

clang-tidy/google/FunctionNamingCheck.cpp

114–125

I'd rather see these diagnostics combined: %select{static|global}1 function name %0 must %select{be in |have an appropriate prefixed followed by "}1 Pascal case as required by the Google Objective-C style guide to simplify the logic (since generateFixItHint() already does the right thing).

[stephanemoore]

Use the select modifier to customize diagnostics for static and non-static function instead of branching diagnostic code paths for static and non-static functions.

[benhamilton]

Thanks, just minor suggestions.

clang-tidy/google/FunctionNamingCheck.cpp

114–118

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.)

115

I know "global" is the correct name, but I feel like "non-static" might be clearer to folks dealing with these error messages.

WDYT?

[stephanemoore]

clang-tidy/google/FunctionNamingCheck.cpp

114–125

I had no idea about the select modifier! Good idea!

test/clang-tidy/google-objc-function-naming.m

8

Sounds like something worth investigating.

Filed https://bugs.llvm.org/show_bug.cgi?id=39941

[stephanemoore]

clang-tidy/google/FunctionNamingCheck.cpp

114–118

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.

115

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++.

[aaron.ballman]

clang-tidy/google/FunctionNamingCheck.cpp

113

Drop the top-level const qualifier, please.

115

How about "%select{static function|function in global namespace}1 named %0..."

[stephanemoore]

Changes:
• Drop const on local bool variable.
• Adopt the term "function in global namespace" in diagnostic messages.

clang-tidy/google/FunctionNamingCheck.cpp

115

Definitely better.

[benhamilton]

clang-tidy/google/FunctionNamingCheck.cpp

115

Yeah, this is better. Thanks.

clang-tidy/google/FunctionNamingCheck.cpp

Finder->addMatcher(
                              matchesName(validFunctionNameRegex(false))))))
           .bind("function"),
       this);
 }
 
 void FunctionNamingCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("function");
 
+  bool IsGlobal = MatchedDecl->getStorageClass() != SC_Static;
   diag(MatchedDecl->getLocation(),
-       "function name %0 not using function naming conventions described by "
-       "Google Objective-C style guide")
-      << MatchedDecl << generateFixItHint(MatchedDecl);
+       "%select{static function|function in global namespace}1 named %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);
 }
 
 } // namespace objc
 } // namespace google
 } // namespace tidy
 } // namespace clang

test/clang-tidy/google-objc-function-naming.m

 // RUN: %check_clang_tidy %s google-objc-function-naming %t
 
 typedef _Bool bool;
 
 static bool ispositive(int a) { return a > 0; }
-// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'ispositive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: static function named 'ispositive'
+// must be in Pascal case as required by Google Objective-C style guide
 // CHECK-FIXES: static bool Ispositive(int a) { return a > 0; }
 
 static bool is_positive(int a) { return a > 0; }
-// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'is_positive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: static function named 'is_positive'
+// must be in Pascal case as required by Google Objective-C style guide
 // CHECK-FIXES: static bool IsPositive(int a) { return a > 0; }
 
 static bool isPositive(int a) { return a > 0; }
-// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'isPositive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: static function named 'isPositive'
+// must be in Pascal case as required by Google Objective-C style guide
 // CHECK-FIXES: static bool IsPositive(int a) { return a > 0; }
 
 static bool Is_Positive(int a) { return a > 0; }
-// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'Is_Positive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: static function named 'Is_Positive'
+// must be in Pascal case as required by Google Objective-C style guide
 // CHECK-FIXES: static bool IsPositive(int a) { return a > 0; }
 
 static bool IsPositive(int a) { return a > 0; }
 
 bool ispalindrome(const char *str);
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'ispalindrome' not using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function in global namespace named
+// 'ispalindrome' must have an appropriate prefix followed by Pascal case as
+// required by Google Objective-C style guide
 
 static const char *md5(const char *str) { return 0; }
-// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: function name 'md5' not using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: static function named 'md5' must be
+// in Pascal case as required by Google Objective-C style guide
 // CHECK-FIXES: static const char *Md5(const char *str) { return 0; }
 
 static const char *MD5(const char *str) { return 0; }
 
 static const char *URL(void) { return "https://clang.llvm.org/"; }
 
 static const char *DEFURL(void) { return "https://clang.llvm.org/"; }
 
 static const char *DEFFooURL(void) { return "https://clang.llvm.org/"; }
 
 static const char *StringFromNSString(id str) { return ""; }
 
 void ABLog_String(const char *str);
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'ABLog_String' not using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function in global namespace named
+// 'ABLog_String' must have an appropriate prefix followed by Pascal case as
+// required by Google Objective-C style guide
 
 void ABLogString(const char *str);
 
 bool IsPrime(int a);
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'IsPrime' not using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function in global namespace named
+// 'IsPrime' must have an appropriate prefix followed by Pascal case as required
+// by Google Objective-C style guide
 
 const char *ABURL(void) { return "https://clang.llvm.org/"; }
 
 const char *ABFooURL(void) { return "https://clang.llvm.org/"; }
 
 int main(int argc, const char **argv) { return 0; }

test/clang-tidy/google-objc-function-naming.mm

 // RUN: %check_clang_tidy %s google-objc-function-naming %t
 
 void printSomething() {}
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'printSomething' not
-// using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function in global namespace named
+// 'printSomething' must have an appropriate prefix followed by Pascal case as
+// required by Google Objective-C style guide
 
 void PrintSomething() {}
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'PrintSomething' not
-// using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function in global namespace named
+// 'PrintSomething' must have an appropriate prefix followed by Pascal case as
+// required by Google Objective-C style guide
 
 void ABCBad_Name() {}
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'ABCBad_Name' not
-// using function naming conventions described by Google Objective-C style guide
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function in global namespace named
+// 'ABCBad_Name' must have an appropriate prefix followed by Pascal case as
+// required by Google Objective-C style guide
 
 namespace {
 
 int foo() { return 0; }
 
 }
 
 namespace bar {