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