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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits