Page MenuHomePhabricator

[clang-format] Extend SpaceBeforeParens for requires
Changes PlannedPublic

Authored by HazardyKnusperkeks on Nov 7 2021, 1:06 PM.

Details

Summary

We can now configure the space between requires and the following paren,separate for clauses and expressions.

Diff Detail

Event Timeline

HazardyKnusperkeks requested review of this revision.Nov 7 2021, 1:06 PM
HazardyKnusperkeks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2021, 1:06 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
curdeius added inline comments.
clang/docs/ClangFormatStyleOptions.rst
3756

You meant "if there is one", right?

clang/unittests/Format/FormatTest.cpp
14306

I'd like to see some tests for mixed true/false, false/true values of these parameters. You only test false/false and true/true combinations so far.

22636

This test seems redundant. Does it test something else than the above added ones?

HazardyKnusperkeks marked 2 inline comments as done.

Addressed comments.

clang/docs/ClangFormatStyleOptions.rst
3756

Yeah

clang/unittests/Format/FormatTest.cpp
22636

No it doesn't, I just thought I would put the requires stuff into the requires test case. I for one work with the gtest filter on single test cases if I work on some issue.

Now the new diff.

MyDeveloperDay accepted this revision.Nov 9 2021, 11:32 AM

LGTM (nit the clang-format check)

This revision is now accepted and ready to land.Nov 9 2021, 11:32 AM
owenpan added inline comments.Nov 9 2021, 5:03 PM
clang/lib/Format/TokenAnnotator.cpp
3145–3148

Nit: remove else.

HazardyKnusperkeks planned changes to this revision.Dec 3 2021, 12:14 PM
HazardyKnusperkeks marked an inline comment as done.

Delayed until D113319 is resolved.

Quuxplusone added inline comments.
clang/docs/ClangFormatStyleOptions.rst
3756

IMO these options should be named InRequiresClause and InRequiresExpression: that's where the space is going. The space doesn't go after the requires-clause. The space doesn't go after the requires-expression.
It occurs to me that the name of this option could be analogous to the name of the option that controls []() {} versus [] () {}... except that it looks like there is no such option (and I'm happy about that). :) Also, the name of that option would probably just be AfterCaptureList, which doesn't help us in this case.

clang/include/clang/Format/Format.h
3371

Here and line 3380, and possibly elsewhere: s/parentheses/parenthesis/

clang/unittests/Format/FormatTest.cpp
14369

Throughout, I'd like to see simpler test cases and more of them. E.g. I think this would suffice, in lieu of lines 14305–14369:

SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresClause = true;
SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresExpression = true;
verifyFormat("void f(auto x) requires (requires (int i) { x+i; }) { }", SpaceAfterRequires);
verifyFormat("if (requires (int i) { x+i; }) return;", SpaceAfterRequires);
verifyFormat("bool b = requires (int i) { x+i; };", SpaceAfterRequires);

SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresClause = true;
SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresExpression = false;
verifyFormat("void f(auto x) requires (requires(int i) { x+i; }) { }", SpaceAfterRequires);
verifyFormat("if (requires(int i) { x+i; }) return;", SpaceAfterRequires);
verifyFormat("bool b = requires(int i) { x+i; };", SpaceAfterRequires);

SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresClause = false;
SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresExpression = true;
verifyFormat("void f(auto x) requires(requires (int i) { x+i; }) { }", SpaceAfterRequires);
verifyFormat("if (requires (int i) { x+i; }) return;", SpaceAfterRequires);
verifyFormat("bool b = requires (int i) { x+i; };", SpaceAfterRequires);

SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresClause = false;
SpaceAfterRequires.SpaceBeforeParensOptions.InRequiresExpression = false;
verifyFormat("void f(auto x) requires(requires(int i) { x+i; }) { }", SpaceAfterRequires);
verifyFormat("if (requires(int i) { x+i; }) return;", SpaceAfterRequires);
verifyFormat("bool b = requires(int i) { x+i; };", SpaceAfterRequires);
clang/docs/ClangFormatStyleOptions.rst
3756

I get your point, but the space does not go anywhere in the clause/expression, so AfterRequiresForClauses?

Quuxplusone added inline comments.Dec 4 2021, 8:20 AM
clang/docs/ClangFormatStyleOptions.rst
3756

(I'd avoid the word For, because of keyword for.)
A requires-expression is the whole expression, requires + param-list + body: https://eel.is/c++draft/expr.prim.req#nt:requires-expression
A requires-clause is the whole clause, requires + logical-or-expr: https://eel.is/c++draft/temp.pre#nt:requires-clause
Does that resolve your concern about the word In?