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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
