hintonda marked an inline comment as done.
hintonda 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>;
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 

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.

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to