[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:17
+
+static const llvm::StringSet<> ValueTraits = {
+    "alignment_of",
----------------
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?


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:104
+
+static const llvm::StringSet<> TypeTraits = {
+    "remove_cv",
----------------
Ditto


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp:105-108
+    "remove_cv",
+    "remove_const",
+    "remove_volatile",
+    "add_cv",
----------------
Please keep alphabetical order.


================
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)
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h:17
+namespace tidy {
+namespace modernize {
+
----------------
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?


================
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();
+
----------------
There should be a space in here, right? Same for the rest.


================
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
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137302