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

Wed Apr 3 08:26:52 PDT 2019

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>;

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60151