[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 libc-commits at lists.llvm.org
Wed Mar 25 02:08:18 PDT 2020

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

  rG LLVM Github Monorepo



More information about the libc-commits mailing list