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 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60151/new/ https://reviews.llvm.org/D60151 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
