njames93 added a comment. This check should only worry about the first declaration of the function, any redecls or the definition could appear outside the namespace like this:
namespace blah{ void foo(); } void blah::foo(); void blah::foo(){} There are some missing test cases I'd like to see like wrapping in an enclosing namespace (including anonymous) namespace __llvm_libc { namespace <optional name> { // impl } } namespace <optional name> { namesapce __llvm_libc { // impl } } As I don't work on libc I'm not sure how these should be handled, maybe its fine if there is a corresponding `inline namespace`. ================ Comment at: clang-tools-extra/clang-tidy/llvmlibc/EntrypointNamespaceCheck.cpp:67 + if (Result.SourceManager->getFilename(MatchedDecl->getLocation()) + .endswith(".h")) + return; ---------------- Is there a rule that all libc implementation headers must have the extension `.h`. If not there is `utils::FileExtensionSet` that could be used. Alternatively you could just check to see if the SourceLocation is in the main file `if (!Result.SourceManager->isInMainFile(MatchedDecl->getLocation())` ================ Comment at: clang-tools-extra/clang-tidy/llvmlibc/EntrypointNamespaceCheck.cpp:71 + std::string RequiredNamespace = + Options.get("RequiredNamespace", "__llvm_libc"); + const DeclContext *EnclosingDecl = ---------------- To save fetching the option each time, why not just store the required namespace in the class and initialize it in the constructor Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76744/new/ https://reviews.llvm.org/D76744 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits