[PATCH] D141892: Implement modernize-use-constraints

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:63-65
+    std::string Name = TheType.getType().getAsString();
+    if (Name.find("enable_if<") == std::string::npos)
+      return std::nullopt;
----------------
This string manipulation is unfortunate, check https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp#L208 for a way to do it without that.
Could extract it out to its own function to use it below as well


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:222
+
+    return std::move(Text);
+  }
----------------
No need for move here.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:235-237
+      const CXXCtorInitializer *FirstInit = *Constructor->init_begin();
+      return utils::lexer::findPreviousTokenKind(FirstInit->getSourceLocation(),
+                                                 SM, LangOpts, tok::colon);
----------------
This seems dangerous, `CXXCtorInitializer` also contains in class initializers
```lang=cpp
struct Foo : Base { // < The first implicit constructor initializer - SourceOrder: -1
  Foo() : 
    Bat(0),  // <- The fourth constructor initializer - SourceOrder: -0
    Baz(0){} // <- The third constructor initializer - SourceOrder: 1
  int Bar = 5; // <- The second (implicit) constructor initializer - SourceOrder: -1
  int Baz = 6;
  int Bat;
};
```
Off the top of my head I can remember if the implicit initializers have a valid source location.
I think tbh the safest way to approach this would actually be to loop through the initializers and check their `GetSourceOrder` for the first item that returns `0`. In the example I have tried to demonstrate the possibilities.


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp:247-248
+        ParamsRange.getEnd(), SM, LangOpts, tok::r_paren, tok::r_paren);
+    return utils::lexer::findNextAnyTokenKind(EndParens, SM, LangOpts,
+                                              tok::equal, tok::equal);
+  }
----------------
Super contrived, but looking for the `= delete` like this could be problematic
```lang=c++
template<typename T>
std::enable_if_t<std::is_const_v<T>> Foo() noexcept(requires (T a) { a = 4; }) = delete;
```
Here I'd imagine it would return the `=` in the noexcept requires clause.
I'd imagine the safest bet would be to work from the function end loc looking for the delete, then the equals.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141892