Page MenuHomePhabricator

[clang-tidy] Add check to ensure llvm-libc implementations are defined in correct namespace.
AbandonedPublic

Authored by PaulkaToast on Mar 24 2020, 7:01 PM.

Diff Detail

Event Timeline

PaulkaToast created this revision.Mar 24 2020, 7:01 PM

Instead of this narrow check, what we really want is a check to ensure that all implementation detail resides in the namespace __llvm_libc. Anything outside would be special and requiring a NOLINT... for them is reasonable. Have you considered such an approach?

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.

clang-tools-extra/clang-tidy/llvmlibc/EntrypointNamespaceCheck.cpp
67

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())

71

To save fetching the option each time, why not just store the required namespace in the class and initialize it in the constructor

PaulkaToast abandoned this revision.Mar 25 2020, 6:30 PM

Abandoning for a more generalized check. D76818

PaulkaToast marked 3 inline comments as done.Mar 25 2020, 6:33 PM
PaulkaToast added inline comments.
clang-tools-extra/clang-tidy/llvmlibc/EntrypointNamespaceCheck.cpp
67

Thanks for this! I incorporated your recommendations in the new patch. (: