This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Remove special logic for parsing concept definitions.
ClosedPublic

Authored by rymiel on Dec 19 2022, 12:43 PM.

Details

Summary

Previously, clang-format relied on a special method to parse concept
definitions, UnwrappedLineParser::parseConcept(), which deferred to
UnwrappedLineParser::parseConstraintExpression(). This is problematic,
because the C++ grammar treats concepts and requires clauses
differently, causing issues such as https://github.com/llvm/llvm-project/issues/55898 and https://github.com/llvm/llvm-project/issues/58130.

This patch removes parseConcept, letting the formatter parse concept
definitions as more like what they actually are, fancy bool definitions.

NOTE that because of this, some long concept definitions change in their
formatting, as can be seen in the changed tests. This is because of a
change in split penalties, caused by a change in MightBeFunctionDecl on
the concept definition line, which was previously true but with this
patch is now false.

One might argue that false is a more "correct" value for concept
definitions, but I'd be fine with setting it to true again to maintain
compatibility with previous versions.

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

Depends on D140330

Diff Detail

Event Timeline

rymiel created this revision.Dec 19 2022, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2022, 12:43 PM
rymiel requested review of this revision.Dec 19 2022, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2022, 12:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rymiel added a project: Restricted Project.Dec 19 2022, 12:43 PM

I'm shocked and impressed, that it just works.

clang/lib/Format/TokenAnnotator.cpp
1683–1684

Does this change anything, besides the penalties and by that the wrapping?

While I can live with both, I think we should not format code differently, if it's not by a bugfix.

clang/unittests/Format/FormatTest.cpp
23945–23946

As I have learned in other changes, we have verifyNoCrash (or similar), this is what should be used. Didn't know about it back then.

rymiel added inline comments.Dec 19 2022, 2:06 PM
clang/lib/Format/TokenAnnotator.cpp
1683–1684

The only changes I could see:

  • the change in Context.IsExpression, which causes the change in penalties;
  • the fact that a new AnnotatedLine isn't created for the body of a concept (i.e. the stuff after the equals sign), where as it previously was.
  • some invalid syntax is formatted differently

It's very likely I don't have a "sample size" large enough.

I am also okay with doing both; I made this issue to gauge what is the preferred direction by the reviewers.

rymiel updated this revision to Diff 484066.Dec 19 2022, 2:10 PM

Use verifyNoCrash

rymiel marked an inline comment as done.Dec 19 2022, 2:10 PM
rymiel added inline comments.
clang/unittests/Format/FormatTest.cpp
23945–23946

Thank you, now I know as well

rymiel marked an inline comment as done.Dec 19 2022, 2:10 PM
HazardyKnusperkeks added inline comments.
clang/lib/Format/TokenAnnotator.cpp
1683–1684

Let's hear from @owenpan @MyDeveloperDay

This revision is now accepted and ready to land.Dec 20 2022, 6:28 AM
rymiel updated this revision to Diff 484985.Dec 22 2022, 4:06 PM

Adapt description of parseConstraintExpression

owenpan accepted this revision.Jan 2 2023, 12:06 AM
owenpan added inline comments.
clang/unittests/Format/FormatTest.cpp
23593–23594

What would happen with parentheses added?

concept DelayedCheck = (static_cast<bool>(0) || requires(T t) { t.bar(); }) && sizeof(T) <= 8;
concept DelayedCheck = static_cast<bool>(0) || (requires(T t) { t.bar(); } && sizeof(T) <= 8);
23946–23947

Just want to make sure that the line break didn't matter.

rymiel added inline comments.Jan 2 2023, 7:40 AM
clang/unittests/Format/FormatTest.cpp
23946–23947

Sorry, that line break was removed when verifyFormat was still used

MyDeveloperDay accepted this revision.Jan 3 2023, 2:44 AM

Thank you for this patch

rymiel marked an inline comment as done.Jan 5 2023, 7:13 PM
rymiel added inline comments.
clang/unittests/Format/FormatTest.cpp
23593–23594

No difference:

concept DelayedCheck =
    (static_cast<bool>(0) || requires(T t) { t.bar(); }) && sizeof(T) <= 8;
concept DelayedCheck =
    static_cast<bool>(0) || (requires(T t) { t.bar(); } && sizeof(T) <= 8);
concept DelayedCheck =
    static_cast<bool>(0) || requires(T t) { t.bar(); } && sizeof(T) <= 8;
rymiel marked an inline comment as done.Jan 5 2023, 7:14 PM
This revision was landed with ongoing or failed builds.Jan 5 2023, 7:18 PM
This revision was automatically updated to reflect the committed changes.