[PATCH] D157239: [clang-tidy] Implement bugprone-incorrect-enable-if

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectEnableIfCheck.cpp:57
+
+  const SourceManager &SM = Result.Context->getSourceManager();
+  SourceLocation RAngleLoc =
----------------
Result got source manager....
https://clang.llvm.org/doxygen/structclang_1_1ast__matchers_1_1MatchFinder_1_1MatchResult.html


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectEnableIfCheck.cpp:61-62
+
+  FixItHint TypenameHint =
+      FixItHint::CreateInsertion(ElaboratedLoc->getBeginLoc(), "typename ");
+  FixItHint TypeHint =
----------------
do we still need typename in C++20 in this context ?
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0634r3.html


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectEnableIfCheck.cpp:67
+  diag(EnableIf->getBeginLoc(), "incorrect std::enable_if usage detected; use "
+                                "'typename std::enable_if<...>::type'")
+      << TypenameHint << TypeHint;
----------------
since C++14 we should recommend using enable_if_t


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:129
+
+  Detects incorrect usages of std::enable_if that don't name the nested 'type'
+  type.
----------------



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/incorrect-enable-if.rst:21
+
+  template <typename T, typename = typename std::enable_if_t<T::some_trait>::type>
+  void valid_usage() { ... }
----------------
shoudnlt be enable_if ?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/incorrect-enable-if.rst:34
+  template <typename T, typename = std::enable_if_t<T::some_trait>>
+  void invalid_usage() { ... }
+
----------------
this is valid usage, uses enable_if_t


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/incorrect-enable-if.rst:37
+  // The tool suggests the following replacement for 'invalid_usage':
+  template <typename T, typename = typename std::enable_if_t<T::some_trait>::type>
+  void invalid_usage() { ... }
----------------
this may not compile, anyway, what if "type" got "type" ?:P


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/incorrect-enable-if.rst:44
+`modernize-type-traits <../modernize/type-traits.html>`_ for another tool
+that will replace ``typename std::enable_if<...>::type`` with
+``std::enable_if_t<...>``, and see
----------------
should be suffucient


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/incorrect-enable-if.rst:47
+`modernize-use-constraints <../modernize/use-constraints.html>`_ for another
+tool that replaces ``std::enable_if`` with C++20 constraints. Use these
+newer mechanisms where possible.
----------------
maybe "Consider" 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157239