Add new check to replace enable_if with C++20 constraints
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
For the sake of demonstration, https://github.com/llvm/llvm-project/commit/9c556ce59edf5a4293d4497d5815544afc0eb878 is the result of running this tool on all headers under clang/include/clang and llvm/include/llvm.
Overall, we could eventually upgrade code in three stages, each a separate reusable check.
- enable_if -> requires clauses
- replace the non _v templates to the _v variants is_same -> is_same_v or the equivalent concept same_as
- replace requires clause on declarations to be template type constraint (replace template <typename T> void foo() requires std::same_as<T, int> void foo() {} to template <std::same_as<int> T> void foo() {}
This is more or less ready for review (not planning on making any further changes; there are more features to be added, but I was thinking of handling those in follow up changesets). I know it's a relatively large review, but let me know if anyone can take a first pass. Thanks!
Would you consider supporting enable_if via parameters
template<typename T> void doStuff(T&, std::enable_if_t<T::some_value, void*> = nullptr) {}
clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp | ||
---|---|---|
72 | nit: Don't need to explicitly call make_optional here, the implicit conversion will handle that for you, same goes for everywhere else. | |
90 | dyn_cast is brought into clangs namespace so the llvm:: qualifier is unnecessary here | |
110–113 | Nit | |
126–129 | Ditto as above | |
130–131 | Don't use else after a return. | |
134 | Prefer pair over tuple when only 2 elements | |
158–161 | Prefer braced initialization rather than the factory make_tuple(or make_pair). Same goes below. | |
216 | This can return a std::optional<StringRef> if you remove the call the .str() and assignment to std::string below. | |
267 | Isn't there a copy of this function in clang::tidy::utils? | |
297–300 | ||
336 | No need to call .str() here if the lambda expects a StringRef, also removed std::make_optional. | |
460–462 | ||
463–467 | Though a different approach of passing the DiagnosticBuilder to the handleReturnType function and just appending the fixits in place would be a nicer solution. Same transformation can be made below. | |
472–475 |
feedback+rebase
clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp | ||
---|---|---|
158–161 | I've seen this feedback twice now - is this (or others, like preferring tuple over pair) written down somewhere? I didn't see anything on https://llvm.org/docs/CodingStandards.htm (or it wasn't obvious to me). Thanks! | |
267 | Ah, this version that I copied takes the Location by modifiable reference so it can be updated. I will update the original version | |
463–467 | Updated to apply your first suggestion. For the addReturnTypeFixes suggestion, I find that handleReturnType returning the vector of FixItHints simpler since it has less responsibility or knowledge about how the FixItHints are to be applied - it's main purpose is to compute them, so why have the function also know how to apply them? Practically speaking, maybe this is not really useful since this function is not say, exposed as a library function to other code (it's just used as an implementation detail of this check). Can you clarify what makes the application in place a nicer solution? |
> Would you consider supporting enable_if via parameters
I was planning to support those too, but in a subsequent commit / review since this review is rather large. Is that OK?
clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp | ||
---|---|---|
63–65 | 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. | |
222 | No need for move here. | |
235–237 | This seems dangerous, CXXCtorInitializer also contains in class initializers 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. | |
247–248 | Super contrived, but looking for the = delete like this could be problematic 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. |
clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp | ||
---|---|---|
247–248 | Good catch. Added a test case for this. Looks like getSourceRange() returns the location after the parameter list and noexcept (if any), but before the = delete, so I went with that. |
Addressed all comments except for the handleReturnType one which I responded to - let me know your thoughts, thanks!
clang-tools-extra/docs/clang-tidy/checks/modernize/use-constraints.rst | ||
---|---|---|
5 |
Overall this check is complicated, simply because it's doing complicated stuff.
- Add description to all functions (what is their purpose, what is expected outcome, maybe put some "example")
- Extend documentation, add "supported" constructions, (pre C++20, C++20).
- What about boost enable_if ? Support it, or restrict check to std only.
- What about enable_if used as an function argument ? Support it, or add some info to documentation that such construction is not supported.
I don't think that this check will ever be "perfect", so LGTM.
We got entire release to "stabilize it".
clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp | ||
---|---|---|
55 | what if we do not have Identifier here ? | |
206–209 | ||
353 | You not checking anywhere if this optional is initialized. | |
clang-tools-extra/docs/clang-tidy/checks/modernize/use-constraints.rst | ||
11 | move this link to new line, 80 characters limit | |
21–25 | Put into documentation also example how this code should look in C++20. |
- What about boost enable_if ? Support it, or restrict check to std only.
- What about enable_if used as an function argument ? Support it, or add some info to documentation that such construction is not supported.
I added some notes to the documentation on supported constructions and unsupported ones. The unsupported ones can be added; I intentionally did not support them in this initial version of the tool to keep the review simpler, although I'd like to add them soon after it lands.
If there are no other comments, @PiotrZSL would you mind landing this for me? Thanks!
clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp | ||
---|---|---|
353 | Indeed - bugprone-unchecked-optional-access finds this! |
clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp | ||
---|---|---|
43 | Would be good to exclude here implicit code. For example with IgnoreUnlessSpelledInSource, maybe in next patch. |
clang-format not found in user’s local PATH; not linting file.