[clang-tidy] Make google-objc-function-naming ignore implicit functions 🙈

[stephanemoore]

Summary

Implicit functions are outside the control of source authors and should
be exempt from style restrictions.

Tested via running clang tools tests.

This is an amended followup to https://reviews.llvm.org/D57207

[stephanemoore]

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

10 ↗

(On Diff #186391)

Thus far I have been unsuccessful in using line markers to simulate this declaration being in a system header but I did discover precedence for using NOLINT to suppress diagnostics in some of the clang-tidy tests:
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/google-runtime-int-std.cpp#L11
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/google-runtime-int.cpp#L6

I think it should be reasonable to suppress the diagnostic here with a comment explaining why. Let me know if you don't think that's an appropriate solution and I can continue investigating for a potential solution using line markers.

[aaron.ballman]

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

10 ↗

(On Diff #186391)

Personally, I would recommend adding stdio.h to extra\test\clang-tidy\Inputs\Headers and adding a -isystem to this test's RUN line. You could also add #pragma clang system_header to the file to be really sure it's treated as a system header. This gives us a place to add more stdio.h declarations in the future as well.

[stephanemoore]

Create a new header stdio.h under //test/clang-tidy/Inputs/Headers/ to contain declaration of
printf and import the new header to google-objc-function-naming.m.

[stephanemoore]

Add a comment to the new stdio.h.

[stephanemoore]

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

10 ↗

(On Diff #186391)

That sounds like a better idea. Thanks for the suggestion!

Things seem to work without needing #pragma clang system_header so I left it out. If you want me to include that, I can do so.

[stephanemoore]

clang-tools-extra/test/clang-tidy/Inputs/Headers/stdio.h

1 ↗

(On Diff #186567)

Do I need the licensing preamble on this test input header?

1–9 ↗

(On Diff #186567)

I noticed that some of the other example headers don't have #ifdef guards at all. I decided to still include them—perhaps mostly for my own sanity. I figured that it wasn't necessary to use the full LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_INPUTS_HEADER_STDIO_H for this header that is mimicking a system header so I went with the shorter _STDIO_H_ which I think better represents what actual stdio.h headers would have. Let me know if you think something else makes more sense.

[aaron.ballman]

LGTM!

clang-tools-extra/test/clang-tidy/Inputs/Headers/stdio.h

1 ↗

(On Diff #186567)

Yes, please add it.

1–9 ↗

(On Diff #186567)

I don't think it matters all that much for our tests; I'm fine with the way you have it currently.

[stephanemoore]

Added licensing preamble to stdio.h

[stephanemoore]

clang-tools-extra/test/clang-tidy/Inputs/Headers/stdio.h

1 ↗

(On Diff #186567)

Added the licensing preamble. Let me know if anything looks amiss.

[aaron.ballman]

Still LGTM, thanks for adding the license snippet!

[stephanemoore]

Thanks for the review!

clang-tidy/google/FunctionNamingCheck.cpp

 
 } // namespace
 
 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.
+  // Enforce Objective-C function naming conventions on all functions except:
+  // • Functions defined in system headers.
+  // • C++ member functions.
+  // • Namespaced functions.
+  // • Implicitly defined functions.
+  // • The main function.
   Finder->addMatcher(
       functionDecl(
           unless(anyOf(isExpansionInSystemHeader(), cxxMethodDecl(),
-                       hasAncestor(namespaceDecl()), isMain(),
+                       hasAncestor(namespaceDecl()), isMain(), isImplicit(),
                        matchesName(validFunctionNameRegex(true)),
                        allOf(isStaticStorageClass(),
                              matchesName(validFunctionNameRegex(false))))))
           .bind("function"),
       this);
 }
 
 void FunctionNamingCheck::check(const MatchFinder::MatchResult &Result) {

test/clang-tidy/Inputs/Headers/stdio.h

+//===--- stdio.h - Stub header for tests ------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _STDIO_H_
+#define _STDIO_H_
+
+// A header intended to contain C standard input and output library
+// declarations.
+
+int printf(const char *, ...);
+
+#endif // _STDIO_H_
+

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

-// RUN: %check_clang_tidy %s google-objc-function-naming %t
+// RUN: %check_clang_tidy %s google-objc-function-naming %t -- -- -isystem %S/Inputs/Headers
+
+#include <stdio.h>
+
+static void TestImplicitFunctionDeclaration(int a) {
+  // Call a builtin function so that the compiler generates an implicit
+  // function declaration.
+  printf("%d", a);
+}
 
 typedef _Bool bool;
 
 static bool ispositive(int a) { return a > 0; }
 // 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; }