[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:172
+void TypeTraitsCheck::registerMatchers(MatchFinder *Finder) {
+  static const ast_matchers::internal::VariadicDynCastAllOfMatcher<
+      Stmt,
----------------
`static` not needed here, remove. I don't think LLVM guidelines enforce strict const correctness either so the `const` could be removed as well if wanted.


================
Comment at: clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h:17
+namespace tidy {
+namespace modernize {
+
----------------
njames93 wrote:
> 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
Sounds good, just want to make sure it's something we see value in fixing (as opposed to just being "churn"). I'm happy to put up an NFC patch for that!


================
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();
+
----------------
njames93 wrote:
> 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.
I disagree, just because the code "compiles" it doesn't mean it's good. The only time I've seen code like this is in [[ https://www.ioccc.org/ | obfuscated code competitions ]] :) 

We should not force the user to run clang-format to fix this - clang-tidy should preserve the existing space. This is not a matter of style either, it's a de-facto way of writting C++ (and many other languages): variable types and variable names in a declaration are separated by a space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137302