This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Implement modernize-use-constraints
ClosedPublic

Authored by ccotter on Jan 16 2023, 10:06 PM.

Details

Summary

Add new check to replace enable_if with C++20 constraints

Diff Detail

Event Timeline

ccotter created this revision.Jan 16 2023, 10:06 PM
Herald added a project: Restricted Project. · View Herald Transcript
ccotter requested review of this revision.Jan 16 2023, 10:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 10:06 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

  1. enable_if -> requires clauses
  2. replace the non _v templates to the _v variants is_same -> is_same_v or the equivalent concept same_as
  3. 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() {}
  1. replace the non _v templates to the _v variants is_same -> is_same_v or the equivalent concept same_as

See D137302

  1. replace the non _v templates to the _v variants is_same -> is_same_v or the equivalent concept same_as

See D137302

Awesome!

ccotter updated this revision to Diff 491127.Jan 21 2023, 9:53 PM
  • Add fno-delayed-template-parsing
ccotter updated this revision to Diff 491201.Jan 22 2023, 12:51 PM
  • Use nested namespaces
ccotter updated this revision to Diff 493050.Jan 28 2023, 6:35 PM

Rebase + Simplify match logic

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.
Same goes for anywhere else that this pattern occurs

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.
Maybe change the function name to addReturnTypeFixes(DiagnosticBuilder&, const FunctionDecl *, const TypeLoc &, const EnableIfData &, ASTContext &);

Same transformation can be made below.

472–475
ccotter updated this revision to Diff 512308.Apr 10 2023, 7:03 PM
ccotter marked 12 inline comments as done.

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?

ccotter updated this revision to Diff 512315.Apr 10 2023, 7:38 PM

arc diff properly

ccotter marked an inline comment as done.Apr 10 2023, 7:39 PM
njames93 added inline comments.Apr 15 2023, 6:49 AM
clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
64–66

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

223

No need for move here.

236–238

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.
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.

248–249

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.
I'd imagine the safest bet would be to work from the function end loc looking for the delete, then the equals.

ccotter updated this revision to Diff 520104.May 6 2023, 10:27 AM
ccotter marked 3 inline comments as done.

Fix bug, other cleanups, rebase

ccotter marked an inline comment as done.May 6 2023, 2:33 PM
ccotter added inline comments.
clang-tools-extra/clang-tidy/modernize/UseConstraintsCheck.cpp
248–249

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.

ccotter updated this revision to Diff 520125.May 6 2023, 2:34 PM
  • Better handling for ctor inits

Addressed all comments except for the handleReturnType one which I responded to - let me know your thoughts, thanks!

bump - @njames93 let me know if you have any further feedback. thanks!

ccotter updated this revision to Diff 545332.Jul 28 2023, 9:59 PM
  • Fix ReleaseNotes again
PiotrZSL retitled this revision from Implement modernize-use-constraints to [clang-tidy] Implement modernize-use-constraints.Jul 29 2023, 7:15 AM
PiotrZSL added inline comments.Jul 29 2023, 7:18 AM
clang-tools-extra/docs/clang-tidy/checks/modernize/use-constraints.rst
5
ccotter updated this revision to Diff 545379.Jul 29 2023, 10:00 AM
  • Fix docs
PiotrZSL accepted this revision.Jul 29 2023, 11:51 AM

Overall this check is complicated, simply because it's doing complicated stuff.

  1. Add description to all functions (what is their purpose, what is expected outcome, maybe put some "example")
  2. Extend documentation, add "supported" constructions, (pre C++20, C++20).
  3. What about boost enable_if ? Support it, or restrict check to std only.
  4. 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.

This revision is now accepted and ready to land.Jul 29 2023, 11:51 AM
ccotter updated this revision to Diff 545450.Jul 30 2023, 10:24 AM
ccotter marked 5 inline comments as done.
  • Improve docs, check for null/empty
ccotter marked an inline comment as done.Jul 30 2023, 10:28 AM
  1. What about boost enable_if ? Support it, or restrict check to std only.
  2. 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!

PiotrZSL accepted this revision.Jul 30 2023, 1:42 PM
PiotrZSL added inline comments.
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.

This revision was automatically updated to reflect the committed changes.
Via Conduit