[clang-tidy] Rename llvm checkers to llvm-project

[hintonda]

Summary

Rename llvm checkers to llvm-project, and namespace from llvm to llvm_project.

Includes renaming llvm directory, many files with llvm- in the name, and updating code and docs.

[alexfh]

The change looks fine, but I don't understand the description of this revision. Could you clarify which checkers you're talking about and which bug you observe?

[aaron.ballman]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

I do not like that we're removing the namespace qualifier here. I would prefer to leave it as ::llvm::SmallSet<::llvm::StringRef, 5> if there is a namespace clash.

[hintonda]

In D60151#1453099, @alexfh wrote:

The change looks fine, but I don't understand the description of this revision. Could you clarify which checkers you're talking about and which bug you observe?

Sorry for not being clearer in my original description.

The error can be triggered by opening up a new llvm namespace under clang::tidy prior to referencing llvm::SmallSet, which would happened if a new llvm checker that gets included in LLVMTidyModule.cpp before HeaderGuardCheck.h -- HeaderGuardCheck.h includes HeaderFileExternsions.h prior to opening the new llvm namespace. Here's the error message you'd get:

llvm-project/clang-tools-extra/clang-tidy/llvm/../utils/../utils/HeaderFileExtensionsUtils.h:24:9: error: no template named 'SmallSet' in namespace 'clang::tidy::llvm'; did you mean '::llvm::SmallSet'?
typedef llvm::SmallSet<llvm::StringRef, 5> HeaderFileExtensionsSet;
        ^~~~~~~~~~~~~~
        ::llvm::SmallSet
llvm-project/llvm/include/llvm/ADT/SmallSet.h:134:7: note: '::llvm::SmallSet' declared here
class SmallSet {
      ^
llvm-project/clang-tools-extra/clang-tidy/llvm/../utils/../utils/HeaderFileExtensionsUtils.h:24:30: error: no member named 'StringRef' in namespace 'clang::tidy::llvm'
typedef llvm::SmallSet<llvm::StringRef, 5> HeaderFileExtensionsSet;
                       ~~~~~~^
2 errors generated.

Here's the one line contrived change needed to demonstrate the error produced above, which is the same effect of including an llvm checker header before including this file:

--- a/clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
+++ b/clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
@@ -16,6 +16,7 @@

 namespace clang {
 namespace tidy {
+namespace llvm {}
 namespace utils {

 typedef llvm::SmallSet<llvm::StringRef, 5> HeaderFileExtensionsSet;

[hintonda]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

Other than aesthetics, the reason I don't like the idea of fully scoping these types, at least without a comment, is that the error is triggered by some other code gets included first, and has nothing to do with this code -- there's nothing actually wrong with the original code. So it could/would be confusing for a reader later on wondering why you needed to fully scope these types, and not others.

[aaron.ballman]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

I would argue that the original code is wrong to not use fully-qualified namespace specifiers. The issue is that we have two different namespaces named llvm and have gotten away with poor namespace hygiene by accident. Either we should rename the clang-tidy llvm namespace to something that does not conflict, or we should consistently use fully-qualified namespace specifiers when in clang-tidy and needing to refer to an llvm namespace explicitly.

I think this patch goes in the wrong direction by making it easier to limp along with poor namespace hygiene.

[hintonda]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

By fully qualified, do you mean appending the global namespace, :: to everything? I actually like using llvm::, but ::llvm:: is odd and needs explanation.

I'd be happy to abandon this change and instead rename the clang::tidy::llvm to clang::tidy::something_else, if that's what the community would prefer.

[alexfh]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

Aaron, you have a very good point. We also have a more recent example of a good namespace hygiene in clang-tidy code: the abseil module is not called absl mainly to "avoid collisions with a well-known top-level namespace" (https://google.github.io/styleguide/cppguide.html#Namespace_Names).

If we can rename the llvm module to something reasonable ("llvm_project"?) without breaking the naming invariants (used by the add_new_check.py script, for example), it would be a much better solution.

[aaron.ballman]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

By fully qualified, do you mean appending the global namespace, :: to everything? I actually like using llvm::, but ::llvm:: is odd and needs explanation.

I mean that within clang-tidy, anywhere we write llvm:: today, we write ::llvm:: instead when we're talking about the global llvm namespace as opposed to the clang-tidy llvm namespace.

I'd be happy to abandon this change and instead rename the clang::tidy::llvm to clang::tidy::something_else, if that's what the community would prefer.

That's my personal preference. I'm fine with the suggestion from @alexfh of using llvm_project instead, but we could also go with llvm_proj, llvm_code, llvm_tidy, etc.

[hintonda]

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

21

Looks like a consensus. I'll work up a patch and get back to you. Thanks again.

[hintonda]

• Rename llvm directory to llvm_project.
• Change llvm- to llvm-project-.
• Rename files.

[alexfh]

In D60151#1454105, @hintonda wrote:

• Rename llvm directory to llvm_project.
• Change llvm- to llvm-project-.
• Rename files.

Awesome! Thanks for doing this. Could you ensure that the add_new_check.py script still works?

[hintonda]

In D60151#1454741, @alexfh wrote:

In D60151#1454105, @hintonda wrote:

• Rename llvm directory to llvm_project.
• Change llvm- to llvm-project-.
• Rename files.

Awesome! Thanks for doing this. Could you ensure that the add_new_check.py script still works?

Thanks for mentioning this. Found a missing rename discovered by add_new_check.py which I'll checkin shortly. However, I'm not sure how best to solve a rename issue.

In rename_check.py, module names can't be hyphenated, e.g.:

190:  old_module = args.old_check_name.split('-')[0]
191:  new_module = args.new_check_name.split('-')[0]

If we keep this pattern, then "llvm-project" won't work. It would need to be something like "llvmproject" or something else. Please let me know your preference.

[aaron.ballman]

In D60151#1454802, @hintonda wrote:

In D60151#1454741, @alexfh wrote:

In D60151#1454105, @hintonda wrote:

• Rename llvm directory to llvm_project.
• Change llvm- to llvm-project-.
• Rename files.

Awesome! Thanks for doing this. Could you ensure that the add_new_check.py script still works?

Thanks for mentioning this. Found a missing rename discovered by add_new_check.py which I'll checkin shortly. However, I'm not sure how best to solve a rename issue.

In rename_check.py, module names can't be hyphenated, e.g.:

190:  old_module = args.old_check_name.split('-')[0]
191:  new_module = args.new_check_name.split('-')[0]

If we keep this pattern, then "llvm-project" won't work. It would need to be something like "llvmproject" or something else. Please let me know your preference.

Hmm, that's unfortunate behavior. I guess my preference would be to go with llvm_project, but it's not a strong preference.

[MyDeveloperDay]

are we supporting "-llvm-*" in existing .clang-tidy files?

If people selectively turn checkers off, won't all of a sudden everyone start getting llvm-project checks and fixes turned back on?

https://github.com/search?q=-llvm-%2A&type=Code

maybe we need to add something like:

llvm-header-guard (redirects to llvm-project-header-guard) <llvm-header-guard>
llvm-header-include-order (redirects to llvm-project-include-order) <llvm-include-order>
llvm-header-namespace-comment(redirects to llvm-project-namespace-comment) <llvm-namespace-comment>
llvm-header-twine-local(redirects to llvm-project-twine-local) <llvm-twine-local>

[hintonda]

In D60151#1462850, @MyDeveloperDay wrote:

are we supporting "-llvm-*" in existing .clang-tidy files?

If people selectively turn checkers off, won't all of a sudden everyone start getting llvm-project checks and fixes turned back on?

https://github.com/search?q=-llvm-%2A&type=Code

maybe we need to add something like:

llvm-header-guard (redirects to llvm-project-header-guard) <llvm-header-guard>
llvm-header-include-order (redirects to llvm-project-include-order) <llvm-include-order>
llvm-header-namespace-comment(redirects to llvm-project-namespace-comment) <llvm-namespace-comment>
llvm-header-twine-local(redirects to llvm-project-twine-local) <llvm-twine-local>

I suppose we could keep the names and directory structure and just change the namespace. That would just be a special case in the scripts. Haven't looked into it yet, but will do so as soon as I can.

[lebedev.ri]

In D60151#1462976, @hintonda wrote:

In D60151#1462850, @MyDeveloperDay wrote:

are we supporting "-llvm-*" in existing .clang-tidy files?

If people selectively turn checkers off, won't all of a sudden everyone start getting llvm-project checks and fixes turned back on?

https://github.com/search?q=-llvm-%2A&type=Code

maybe we need to add something like:

llvm-header-guard (redirects to llvm-project-header-guard) <llvm-header-guard>
llvm-header-include-order (redirects to llvm-project-include-order) <llvm-include-order>
llvm-header-namespace-comment(redirects to llvm-project-namespace-comment) <llvm-namespace-comment>
llvm-header-twine-local(redirects to llvm-project-twine-local) <llvm-twine-local>

I suppose we could keep the names and directory structure and just change the namespace. That would just be a special case in the scripts. Haven't looked into it yet, but will do so as soon as I can.

Isn't that matching done on strings? I.e. is there difference between -llvm-* and -ll* ?

[MyDeveloperDay]

I suppose we could keep the names and directory structure and just change the namespace. That would just be a special case in the scripts. Haven't looked into it yet, but will do so as soon as I can.

Isn't that matching done on strings? I.e. is there difference between -llvm-* and -ll* ?

it would be if they only do it to -llm-* (which could possibly catch most cases)

but what if someone is doing

-llvm-header-guard

We wouldn't catch those..

This is a snippet of a .clang-tidy file from a project I work on, we turn off specific checks by fully qualifying the checker

....
llvm-*,
-llvm-header-guard,
-llvm-include-order,
misc-*,
modernize-*,
...

[hintonda]

In D60151#1463484, @MyDeveloperDay wrote:

I suppose we could keep the names and directory structure and just change the namespace. That would just be a special case in the scripts. Haven't looked into it yet, but will do so as soon as I can.

Isn't that matching done on strings? I.e. is there difference between -llvm-* and -ll* ?

it would be if they only do it to -llm-* (which could possibly catch most cases)

but what if someone is doing

-llvm-header-guard

We wouldn't catch those..

This is a snippet of a .clang-tidy file from a project I work on, we turn off specific checks by fully qualifying the checker

....
llvm-*,
-llvm-header-guard,
-llvm-include-order,
misc-*,
modernize-*,
...

You make a great point. I'll look into just changing the namespace name. I wasn't really comfortable with all the code churn it involved anyway, but once I got started, I figured I'd finish it. The duplicate llvm namespace is the issue, not other names. Thanks for the feedback...

[hintonda]

Replaced by D60629.

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h

 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
 namespace tidy {
 namespace utils {
 
-typedef llvm::SmallSet<llvm::StringRef, 5> HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = SmallSet<StringRef, 5>;
 
 /// \brief Checks whether expansion location of \p Loc is in header file.
 bool isExpansionLocInHeaderFile(
     SourceLocation Loc, const SourceManager &SM,
     const HeaderFileExtensionsSet &HeaderFileExtensions);
 
 /// \brief Checks whether presumed location of \p Loc is in header file.
 bool isPresumedLocInHeaderFile(

clang/include/clang/Basic/LLVM.h

 namespace llvm {
   // ADT's.
   class StringRef;
   class Twine;
   class VersionTuple;
   template<typename T> class ArrayRef;
   template<typename T> class MutableArrayRef;
   template<typename T> class OwningArrayRef;
+  template<typename T, unsigned N, typename C> class SmallSet;
   template<unsigned InternalLen> class SmallString;
   template<typename T, unsigned N> class SmallVector;
   template<typename T> class SmallVectorImpl;
   template<typename T> class Optional;
   template <class T> class Expected;
 
   template<typename T>
   struct SaveAndRestore;
namespace clang {
 
   // ADT's.
   using llvm::ArrayRef;
   using llvm::MutableArrayRef;
   using llvm::None;
   using llvm::Optional;
   using llvm::OwningArrayRef;
   using llvm::SaveAndRestore;
+  using llvm::SmallSet;
   using llvm::SmallString;
   using llvm::SmallVector;
   using llvm::SmallVectorImpl;
   using llvm::StringRef;
   using llvm::Twine;
   using llvm::VersionTuple;
 
   // Error handling.