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: > 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. Looks like a consensus. I'll work up a patch and get back to you. Thanks again. 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
