[clang-tidy] Ignore namespaced and C++ member functions in google-objc-function-naming check 🙈

[stephanemoore]

Summary

The google-objc-function-naming check applies to functions that are not namespaced and should not be applied to C++ member functions. Such function declarations should be ignored by the check to avoid false positives in Objective-C++ sources.

[aaron.ballman]

LGTM!

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

24–25 ↗

(On Diff #176007)

Can you run this test file through clang-format?

[benhamilton]

IMHO we should just disable this check entirely for C++ files (including Objective-C++).

Since Objective-C++ files will have a mix of the Google Objective-C and Google C++ styles, I think there will be too many places where we'll hit conflicts like this (there are likely more).

[stephanemoore]

Re-formatted google-objc-function-naming.mm to LLVM style.

[stephanemoore]

In D55101#1314716, @benhamilton wrote:

IMHO we should just disable this check entirely for C++ files (including Objective-C++).

Since Objective-C++ files will have a mix of the Google Objective-C and Google C++ styles, I think there will be too many places where we'll hit conflicts like this (there are likely more).

Would you be okay with landing this fix and if we get any further reports for Objective-C++ sources then we can suppress it in all C++/Objective-C++ sources? I think there is enough value to enforcing the naming conventions on non-namespaced C functions in Objective-C++ to justify a simple followup fix. If other issues are reported after this then I also agree that enforcement in Objective-C++ sources may incur more overhead than it's worth.

[benhamilton]

Would you be okay with landing this fix and if we get any further reports for Objective-C++ sources then we can suppress it in all C++/Objective-C++ sources? I think there is enough value to enforcing the naming conventions on non-namespaced C functions in Objective-C++ to justify a simple followup fix. If other issues are reported after this then I also agree that enforcement in Objective-C++ sources may incur more overhead than it's worth.

I'm not against it, but we've already disabled the majority of Objective-C checks for Objective-C++ code, so I don't think this one should apply either.

[stephanemoore]

In D55101#1315294, @benhamilton wrote:

Would you be okay with landing this fix and if we get any further reports for Objective-C++ sources then we can suppress it in all C++/Objective-C++ sources? I think there is enough value to enforcing the naming conventions on non-namespaced C functions in Objective-C++ to justify a simple followup fix. If other issues are reported after this then I also agree that enforcement in Objective-C++ sources may incur more overhead than it's worth.

I'm not against it, but we've already disabled the majority of Objective-C checks for Objective-C++ code, so I don't think this one should apply either.

Within the module named "google-module", I only see that AvoidCStyleCastsCheck is disabled for Objective-C++. That check is probably disabled because C-style casts on Objective-C types in Objective-C++ code are not seen as a major issue and the cost of refactoring code to use or not use C-style casts as code is refactored between Objective-C and Objective-C++ sources might be considered high.
Within the module named "objc-module", it appears that all checks are currently enabled for Objective-C++ code.
Looking across all the clang-tidy modules, AvoidCStyleCastsCheck might actually be the only check that is specifically disabled for Objective-C++ (although the grep I used to posit this assumes that any check that is suppressed in Objective-C++ must contain the string getLangOpts().ObjC: grep -r "getLangOpts().ObjC" clang-tidy/).

There are certainly cases where the heterogeneity of Objective-C++ encourages disabling a check due to inability to distinguish between whether C++ or Objective-C practices should be contextually dominant. I think that google-objc-function-naming should be able to cleanly separate declarations that should follow C++ or Objective-C patterns. If further bugs are reported after this is submitted then I am more willing to concede that I was overly optimistic. If it's alright with you, I would prefer to land this fix to give google-objc-function-naming another attempt at proper enforcement in Objective-C++ and consider disabling it in all C++ variants as a followup to further bug reports in Objective-C++.

[aaron.ballman]

In D55101#1315294, @benhamilton wrote:

Would you be okay with landing this fix and if we get any further reports for Objective-C++ sources then we can suppress it in all C++/Objective-C++ sources? I think there is enough value to enforcing the naming conventions on non-namespaced C functions in Objective-C++ to justify a simple followup fix. If other issues are reported after this then I also agree that enforcement in Objective-C++ sources may incur more overhead than it's worth.

I'm not against it, but we've already disabled the majority of Objective-C checks for Objective-C++ code, so I don't think this one should apply either.

I don't do a lot of Objective-C programming, so take my perspective with the giant grain of salt it deserves. I think this is a reasonable incremental improvement because this code seems more Cish than C++ish, so I can see an argument being made for following the Objective-C rules more than the C++ rules in this one instance. I think it's reasonable to push this commit and then revisit the question for the google module as a whole if there are conflicts between the rules in the same module.

[stephanemoore]

Chatted offline with Ben and confirmed that he's okay with proceeding.

clang-tools-extra/trunk/clang-tidy/google/FunctionNamingCheck.cpp

void FunctionNamingCheck::registerMatchers(MatchFinder *Finder) {
   // This check should only be applied to Objective-C sources.
   if (!getLangOpts().ObjC)
     return;
 
   // Match function declarations that are not in system headers and are not
   // main.
   Finder->addMatcher(
       functionDecl(
-          unless(isExpansionInSystemHeader()),
-          unless(anyOf(isMain(), matchesName(validFunctionNameRegex(true)),
+          unless(anyOf(isExpansionInSystemHeader(), cxxMethodDecl(),
+                       hasAncestor(namespaceDecl()), isMain(),
+                       matchesName(validFunctionNameRegex(true)),
                        allOf(isStaticStorageClass(),
                              matchesName(validFunctionNameRegex(false))))))
           .bind("function"),
       this);
 }
 
 void FunctionNamingCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("function");

clang-tools-extra/trunk/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
+
+void PrintSomething() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'PrintSomething' not
+// using function naming conventions described 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
+
+namespace {
+
+int foo() { return 0; }
+
+}
+
+namespace bar {
+
+int convert() { return 0; }
+
+}
+
+class Baz {
+public:
+  int value() { return 0; }
+};