[PATCH] D137302: [clang-tidy] Add modernize-type-traits check

[Nathan James via Phabricator via cfe-commits]

njames93 marked 2 inline comments as done.
njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:140
+const internal::VariadicDynCastAllOfMatcher<TypeLoc, DependentNameTypeLoc>
+    dependentNameTypeLoc; // NOLINT(readability-identifier-naming)
+
----------------
carlosgalvezp wrote:
> I don't see a reason why the naming convention can't be followed here?
Following the naming convention of the AST matchers, which all use camelBack


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:142
+
+namespace internal {
+DeclarationName getName(const DependentScopeDeclRefExpr &D) {
----------------
carlosgalvezp wrote:
> Why is an internal namespace needed, when you are already have the anonymous namespace?
I think it was because the matchers use an internal namespace, probably not need and can be removed. Let me check.


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:181
+  // Only register matchers for trait<...>::value in c++17 mode.
+  if (getLangOpts().CPlusPlus17) {
+    Finder->addMatcher(mapAnyOf(declRefExpr, dependentScopeDeclRefExpr)
----------------
carlosgalvezp wrote:
> I think clang-format should remove these braces since it's only 1 statement in the "if" condition. 
As its more than 1 lines, keeping it in braces is cleaner.
Also clang-format passed on the pre-merge so it thinks this code is correct according to the style guide


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h:31
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
+  llvm::Optional<TraversalKind> getCheckTraversalKind() const override;
+};
----------------
carlosgalvezp wrote:
> There's an ongoing effort to deprecate llvm::Optional in favor of std::optional, perhaps start using it right away in this check?
This patch was last update before the switch, when the virtual function still returned an llvm::Optional, when I rebase it will need changing.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/type-traits.cpp:23
+// CHECK-MESSAGES-CXX17: :[[@LINE-1]]:19: warning: use c++17 style variable templates
+// CHeCK-FIXES-CXX17: bool NoTemplate = std::is_const_v<bool>
+
----------------
carlosgalvezp wrote:
> Typo. Shouldn't the test have failed?
Good catch, it won't fail the test as filecheck will just ignore it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137302