This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] AllowShortCompoundRequirementOnASingleLine

Authored by Backl1ght on Dec 12 2022, 4:35 AM.



clang-format brace wrapping did not take requires into consideration, compound requirements will be affected by BraceWrapping.AfterFunction.

I plan to add 3 options AllowShortRequiresExpressionOnASingleLine, AllowShortCompoundRequirementOnASingleLine and BraceWrapping.AfterRequiresExpression.

This patch is for AllowShortCompoundRequirementOnASingleLine.

Diff Detail

Event Timeline

Backl1ght created this revision.Dec 12 2022, 4:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 4:35 AM
Backl1ght requested review of this revision.Dec 12 2022, 4:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2022, 4:35 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Backl1ght edited the summary of this revision. (Show Details)Dec 12 2022, 6:11 AM

See the comments on D139786. And you can base your commits on each other, and then add a child rsp. parent revision here in phabricator, that will prevent merge conflicts.


haven't we use "Requires" in other options? What is the definition of Compound?

I might be tempted for this to be AllShortRequiresOnASingleLine but then it be an enum with the following options


why would the default be true, is that what happens today?

Backl1ght added inline comments.Dec 13 2022, 7:06 AM

There are some options related to requires like IndentRequiresClause, RequiresClausePosition, etc. But as for single line and brace wrapping, requires is not yet considered.

"Compound" is from Compound Requirements section in this page

I think this is a better way, I would try implementing it.



MyDeveloperDay added inline comments.Dec 13 2022, 9:30 AM

just to clarify so I'm sure (without me having to try it myself), if you hadn't introduce this option it would be the equivalent of true.

The only reason I say is we get complained at when we change the default from not doing something to doing something. Even if that means before we left it alone.

So mostly we normally find the options go through an evolution from bool->enum->struct, sometimes it can be better to introduce an enum so we can have "Leave" as the default

in such circumstances you let the old behaviour be the default, that way we know. That previously unformatted compound statements won't be touch in any way.

else if (Style.AllowShortCompoundRequirementOnASingleLine != Leave  && .......)

Users then have to "positively" buy into your style change one way or another. Rather than us imposing a default even if that default seems perfectly reasonable.


The true as default is correct.

MyDeveloperDay added inline comments.Dec 14 2022, 8:33 AM

Then if @HazardyKnusperkeks you think this is ok, I'm ok with it too.

rymiel added inline comments.Mar 11 2023, 12:11 AM

A compound-requirement is what the standard calls it, so I think the name is fine.

I'm not sure if collating it with other options as suggested is a good idea, since a requires-expression can contain multiple compound-requirements, such as:

requires {
  { E1 } -> C;
  { E2 } -> D<A1>;

This requires-expression has two compound-requirements, and two formatting of those two individually is what's being controlled by the option.

Although I will agree it should probably still be an enum, but in that case, it probably shouldn't start with "Allow"

rymiel accepted this revision.May 15 2023, 2:30 PM

I see nothing wrong with this patch alone as it currently stands, since it's a quite simple change to the LineJoiner, and I see it as one of the final stopgaps in clang-format's support for requires clauses, and I think it definitely beats the current buggy behavior where behaviour is conflated with BraceWrapping.AfterFunction.
But I'd like other reviewers to maybe have another look?

This revision is now accepted and ready to land.May 15 2023, 2:30 PM
This revision was landed with ongoing or failed builds.Oct 25 2023, 5:17 AM
This revision was automatically updated to reflect the committed changes.