[PATCH] D60151: [clang-tidy] Add using SmallSet to LLVM.h to fix bug in typedef in llvm checkers.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet<llvm::StringRef, 5> HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = SmallSet<StringRef, 5>;
 
----------------
alexfh wrote:
> hintonda wrote:
> > aaron.ballman wrote:
> > > hintonda wrote:
> > > > aaron.ballman wrote:
> > > > > 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.
> > > > 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.
> > > 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.
> > 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.
> > 
> 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.
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60151/new/

https://reviews.llvm.org/D60151