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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.


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


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:142
+
+namespace internal {
+DeclarationName getName(const DependentScopeDeclRefExpr &D) {
----------------
Why is an internal namespace needed, when you are already have the anonymous namespace?


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:143
+namespace internal {
+DeclarationName getName(const DependentScopeDeclRefExpr &D) {
+  return D.getDeclName();
----------------
Internal functions and variables should be declared "static", outside the anonymous namespace, as per the LLVM Coding Standards. AST matchers can be put in the anonymous namespace.


================
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)
----------------
I think clang-format should remove these braces since it's only 1 statement in the "if" condition. 


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h:30
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
+  llvm::Optional<TraversalKind> getCheckTraversalKind() const override;
----------------
Nit: typically this is implemented inline in the header.


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h:31
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
+  llvm::Optional<TraversalKind> getCheckTraversalKind() const override;
+};
----------------
There's an ongoing effort to deprecate llvm::Optional in favor of std::optional, perhaps start using it right away in this check?


================
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>
+
----------------
Typo. Shouldn't the test have failed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137302