Page MenuHomePhabricator

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

Authored by hintonda on Apr 2 2019, 2:07 PM.

Details

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.

Event Timeline

hintonda created this revision.Apr 2 2019, 2:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 2:07 PM
Herald added a subscriber: xazax.hun. · View Herald Transcript
alexfh accepted this revision.Apr 3 2019, 6:20 AM

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?

This revision is now accepted and ready to land.Apr 3 2019, 6:20 AM
aaron.ballman requested changes to this revision.Apr 3 2019, 6:28 AM
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21 ↗(On Diff #193362)

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.

This revision now requires changes to proceed.Apr 3 2019, 6:28 AM

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 marked an inline comment as done.Apr 3 2019, 8:21 AM
hintonda added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21 ↗(On Diff #193362)

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 added inline comments.Apr 3 2019, 8:26 AM
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21 ↗(On Diff #193362)

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 marked an inline comment as done.Apr 3 2019, 8:53 AM
hintonda added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21 ↗(On Diff #193362)

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 added inline comments.Apr 3 2019, 10:27 AM
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21 ↗(On Diff #193362)

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 added inline comments.Apr 3 2019, 10:36 AM
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21 ↗(On Diff #193362)

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 marked an inline comment as done.Apr 3 2019, 10:45 AM
hintonda added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21 ↗(On Diff #193362)

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

hintonda updated this revision to Diff 193604.Apr 3 2019, 2:59 PM
  • Rename llvm directory to llvm_project.
  • Change llvm- to llvm-project-.
  • Rename files.
hintonda retitled this revision from [clang-tidy] Add using SmallSet to LLVM.h to fix bug in typedef in llvm checkers. to [clang-tidy] Rename llvm checkers to llvm-project .Apr 3 2019, 3:03 PM
hintonda edited the summary of this revision. (Show Details)
alexfh added a comment.Apr 4 2019, 5:32 AM
  • 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?

  • 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.

  • 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.

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>

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.

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* ?

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-*,
...

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 abandoned this revision.Apr 12 2019, 12:41 PM

Replaced by D60629.