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

[Nathan James via Phabricator via cfe-commits]

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


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:17
+
+static const llvm::StringSet<> ValueTraits = {
+    "alignment_of",
----------------
carlosgalvezp wrote:
> I'm guessing this list was obtained in some automatic way - could we document the process for generating it, so a future maintainer knows how to do it?
They weren't, just copied from cppreference, I'm sure there is a way to automate it, but honestly there's not much point


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:105-108
+    "remove_cv",
+    "remove_const",
+    "remove_volatile",
+    "add_cv",
----------------
carlosgalvezp wrote:
> Please keep alphabetical order.
This is the order they appear on cppreference, so I think it makes sense to keep it like that, will make for an easier time maintaining this


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:134-139
+static const internal::VariadicDynCastAllOfMatcher<Stmt,
+                                                   DependentScopeDeclRefExpr>
+    dependentScopeDeclRefExpr; // NOLINT(readability-identifier-naming)
+static const internal::VariadicDynCastAllOfMatcher<TypeLoc,
+                                                   DependentNameTypeLoc>
+    dependentNameTypeLoc; // NOLINT(readability-identifier-naming)
----------------
carlosgalvezp wrote:
> Why do we need these matchers at global scope, if they are only used in the `registerMatchers` function?
> 
> Unless they are POD types, putting objects at global scope is bugprone since they are initialized before `main` in an unspecified order.
These are stateless matchers which require no construction, so there will be no initialisation order issues.

They could be moved into the function that uses them without any issue though.


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h:17
+namespace tidy {
+namespace modernize {
+
----------------
carlosgalvezp wrote:
> njames93 wrote:
> > Eugene.Zelenko wrote:
> > > tschuett wrote:
> > > > Why do you refuse to use nested namespaces in `modernize` ?
> > > I think this is done for legacy reasons. This should be done for all checks and `add_new_check.py`.
> > Because the add_new_check script hasn't been updated since we went to c++17
> Is the goal to update all existing checks to C++17 format?
It should be done separately as an NFC change


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/type-traits.cpp:38
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use c++14 style type templates
+// CHECK-FIXES: std::enable_if_t<b>inTemplate();
+
----------------
carlosgalvezp wrote:
> There should be a space in here, right? Same for the rest.
I'm assuming you mean between `<b>` and `inTemplate`.
If so, because we disable clang-format in the tests, the code isn't being reformatted and the replacement logic happens to get rid of the space too. As missing the space doesn't break the code and clang-format would reformat the code for us in typical use cases, its not worth worrying about.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/type-traits.cpp:52
+
+// For macros we don't want any fixes to be emitted
+
----------------
carlosgalvezp wrote:
> As a user I find it a bit confusing that there is a warning, but no fix. Should we just disable the warning on macros entirely? 
> 
> Furthermore, the warning shows up at the call site, so if it's not possible to fix it will be very noisy for people to suppress it everywhere where the macro is used. Maybe add an option `IgnoreMacros` similar to how it's done for other checks? Alternatively, as a user I would like to be able to suppress the warning in line 54 once only instead of at the call site.
I think `IgnoreMacros` is probably the best approach here, Getting warnings about marcos can be helpful if they are able to be fixed, but it would be nice to also silence those in cases when they can't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137302