Page MenuHomePhabricator

[clang-format] Concepts: allow identifiers after negation
ClosedPublic

Authored by rymiel on Aug 16 2022, 10:04 AM.

Details

Summary

Previously, the formatter would refuse to treat identifiers within a
compound concept definition as actually part of the definition, if
they were after the negation operator !. It is now made consistent
with the likes of && and ||.

Fixes https://github.com/llvm/llvm-project/issues/55898

Diff Detail

Event Timeline

rymiel created this revision.Aug 16 2022, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2022, 10:04 AM
rymiel requested review of this revision.Aug 16 2022, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2022, 10:04 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rymiel added a project: Restricted Project.Aug 16 2022, 10:06 AM

Nice patch. Could you please also extend the test in TokenAnnotatorTests.cpp?

rymiel updated this revision to Diff 453107.EditedAug 16 2022, 1:12 PM

Added negation unary operator check to TokenAnnotatorTest

Is this what you meant? Although, I didn't touch the TokenType since it was already correct before i changed anything

Added negation unary operator check to TokenAnnotatorTest

Is this what you meant? Although, I didn't touch the TokenType since it was already correct before i changed anything

It's late (where I am). I thought we had something like ClosesRequiresClause for concepts too.
But on the other hand, this should affect requires clauses, right? So a test for that would fail before your patch.

This revision is now accepted and ready to land.Aug 16 2022, 1:37 PM

It's late (where I am). I thought we had something like ClosesRequiresClause for concepts too.
But on the other hand, this should affect requires clauses, right? So a test for that would fail before your patch.

Sorry, I'm not familiar with these tests enough to know what exactly you mean, but thank you for the review
Also, since this is my first change, I cannot commit this myself.

It seems that even with this patch, there is seemingly weird formatting with the negation in requires clauses, such as:

template <typename T>
  requires !F<T>
           int bar(T t);

This is because the "fake parens" of the unary expression opened is never closed by the special case clean-up made for requires clauses
However, it turns out this isn't even valid syntax, as it requires parentheses for disambiguation. Adding those brings back correct indentation

template <typename T>
  requires(!F<T>)
int bar(T t);

Is it okay to ignore this case, as it wouldn't even compile?

It seems that even with this patch, there is seemingly weird formatting with the negation in requires clauses, such as:

template <typename T>
  requires !F<T>
           int bar(T t);

This is because the "fake parens" of the unary expression opened is never closed by the special case clean-up made for requires clauses
However, it turns out this isn't even valid syntax, as it requires parentheses for disambiguation. Adding those brings back correct indentation

template <typename T>
  requires(!F<T>)
int bar(T t);

Is it okay to ignore this case, as it wouldn't even compile?

Yeah it is okay when invalid code is not formatted "nicely". It wouldn't hurt if we do, but the time can be invested better.

We'll give the others some time to react, I will (most likely) push the commit on the weekend.

owenpan accepted this revision.Aug 17 2022, 10:37 PM
curdeius accepted this revision.EditedMon, Sep 5, 12:32 AM

@rymiel, please provide your name and email address for the commit message, so that we can land it for you.

Please state a name and email for the commit.

This revision was landed with ongoing or failed builds.Mon, Sep 5, 3:36 AM
This revision was automatically updated to reflect the committed changes.