[libc-commits] [PATCH] D76744: [clang-tidy] Add check to ensure llvm-libc implementations are defined in correct namespace.

[Nathan James via Phabricator via libc-commits]

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