[PATCH] D135404: [clang-tidy] Add a checker for converting into C++17 variable template type traits

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson marked 2 inline comments as done.
royjacobson added a comment.

In D135404#3842156 <https://reviews.llvm.org/D135404#3842156>, @njames93 wrote:

> Why limit this to just the value type traits, makes sense to also support type type traits.

I'd love to, but I can't match against the `::type` usages with `DeclRefExpr`. I think I'd have to match all types in the program or something like that, but I'm not sure if even that's enough because of unevaluated contexts etc... So I decided to focus on this first.



================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:73
+    const auto *RecordQualifier =
+        cast_if_present<ClassTemplateSpecializationDecl>(
+            Qualifier->getAsRecordDecl());
----------------
njames93 wrote:
> Won't this assert if a pointer is supplied but it isn't a ClassTemplateSpecializationDefl. Wouldn't dyn_cast be needed.
> In any case a test where you access a static variable from a non template class needs adding to make sure no warning is emitter.
Definitely need a dyn_cast here, thanks!


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:86
+    auto CurrentText = tooling::fixit::getText(*MatchedDecl, *Result.Context);
+    if (replacement_regex.match(CurrentText))
+      IssuedDiag << FixItHint::CreateReplacement(
----------------
njames93 wrote:
> This fixit logic looks cumbersome, just replace the qualifier loc(::)to the declared with an `_v`. But only iff the qualified name loc is not a macro.
I think to do that I have to get the location of the template-id of the qualifier from the ClassTemplateSpecializationDecl, but I haven't really managed to do that. Or is there maybe a way to get the lexer tokens list of a source range or something like that?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize/type-traits.rst:28
+
+.. option:: ValueTypeTraits
+
----------------
njames93 wrote:
> I'm not sure this option would ever be used in the wild as it would override all the default values in the stl.
The intended use case is one-time running of this check over a code base to modernize it, and you want to add your own type traits that you define across your own libraries. I think that's useful, at least.

Maybe this should just _add_ traits, and not replace them? Is this something that clang-tidy does? I basically copied this from the `modernize-use-emplace` check.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135404