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
