Page MenuHomePhabricator

[clang-tidy] Add check llvmlibc-implementation-in-namespace.
ClosedPublic

Authored by PaulkaToast on Mar 25 2020, 6:25 PM.

Details

Summary

This check makes sure all llvm-libc implementations falls within the __llvm_libc namespace.

The check is quite general, lots of projects do this by conversion to avoid pollution. Not sure if there is a desire to put this in a different module?

Diff Detail

Event Timeline

PaulkaToast created this revision.Mar 25 2020, 6:25 PM

Please also add isLanguageVersionSupported(), because namespaces make sense only in C++.

sivachandra added inline comments.Mar 25 2020, 10:21 PM
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
31

For completeness, can you include a nested namespace and a function declaration here?

If you want to make it a general check, you should consider making the default module options set the correct namespace
RequiredNamespace

ClangTidyOptions getModuleOptions() override {
  ClangTidyOptions Options;
  Options.CheckOptions["llvmlibc-implementation-in-namespace.RequiredNamespace"] = "__llvm_libc";
  return Options;
}
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
7

Small nit : Not a fan of the diagnostic saying CXX Record. Maybe a pain but getDeclKindName isn't the best to expose to users.

Updated to handle nested namespaces, exclude C linkages functions, and made check language specific. (:

PaulkaToast marked 3 inline comments as done.Mar 27 2020, 4:48 PM

If you want to make it a general check, you should consider making the default module options set the correct namespace
RequiredNamespace

Changed the check a bit so I'm not sure if pulling it out is still something desired, if so what module do you recommend this should live under?

clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
7

Removed getDeclKindName as recommended.

PaulkaToast marked an inline comment as done.Mar 31 2020, 3:35 AM

Friendly Ping @njames93 (:
Any other changes needed?

sivachandra added inline comments.Mar 31 2020, 10:14 PM
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
17

Can you add __llvm_libc as a nested namespace here.

PaulkaToast marked an inline comment as done.

This check needs a store Options override method as it reading options from a configuration. Check other checks to see how this is done.

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h
36

This can be made const

aaron.ballman added inline comments.Apr 3 2020, 10:45 AM
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
22

This skips linkage spec declarations, but are there other declarations that should be similarly skipped? For instance static_assert declarations?

34–35

Instead of doing an isa<> followed by a cast<>, the more common pattern is to do:

if (const auto *NS = dyn_cast<NamespaceDecl>(MatchedDecl)) {
37

clang-tidy diagnostics don't have punctuation, so you should drop the full stop.

43

They also aren't grammatically correct sentences, so the capital P and period should both go. While this definitely gets points for politeness, I think a more typical diagnostic might be: declaration must be declared within the '%0' namespace

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h
36

Will there only ever be a single namespace? Or should this be a list (for instance, a main namespace and a details namespace)?

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
7

Checks that all declarations in the llvm-libc implementation are within the correct namespace.

33–36

Given that this check is specific to llvm-libc, why is the option needed at all?

PaulkaToast marked 13 inline comments as done.

Addressed njames's and aaron's comments. (:

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
22

I believe that linkage is the only exception needed, static_asserts and all other declarations should also be within the namespace.

34–35

Ah thank you this looks much nicer!

43

ah, alright, good call. (:

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h
36

There will only be this one namespace.

clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
33–36

I was concerned that maybe there would be a desire to make the check generalized, however it does seem quite specific so I will take your advice.

aaron.ballman accepted this revision.Apr 6 2020, 9:01 AM

LGTM!

clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
22

Excellent, thank you for verifying!

This revision is now accepted and ready to land.Apr 6 2020, 9:01 AM
This revision was automatically updated to reflect the committed changes.