Page MenuHomePhabricator

[clang-format][c++2b] support removal of the space between auto and {} in P0849R8
ClosedPublic

Authored by MyDeveloperDay on Sat, Nov 13, 6:35 AM.

Details

Summary

Looks like the work of D113393: [c++2b] Implement P0849R8 auto(x) requires manual clang-formatting intervention.

Removal of the space between auto and {}

@lichray would this help?

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Sat, Nov 13, 6:35 AM
MyDeveloperDay created this revision.
Quuxplusone requested changes to this revision.Sat, Nov 13, 7:12 AM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
clang/unittests/Format/FormatTest.cpp
22574

Looks like a good improvement to me!
However, note that auto(), auto{}, auto({}), auto{{}} are all invalid expressions; and it would be good to test with something inside the braces anyway. Please replace with e.g.

verifyFormat("foo(auto(1))");
verifyFormat("foo(auto{1})");
verifyFormat("foo(new auto(1))");
verifyFormat("foo(new auto{1})");
verifyFormat("decltype(auto(1)) x;");
verifyFormat("decltype(auto{1}) x;");
verifyFormat("auto(x);");
verifyFormat("auto{x};");
verifyFormat("auto{x} = y;");
verifyFormat("auto(x) = y;");  // actually a declaration, but this is clearly the user's own fault
verifyFormat("integral auto(x) = y;");  // actually a declaration, but this is clearly the user's own fault
verifyFormat("auto(*p)() = f;");  // actually a declaration; TODO FIXME
This revision now requires changes to proceed.Sat, Nov 13, 7:12 AM

Additional use cases

Quuxplusone accepted this revision.Sat, Nov 13, 7:40 AM

Personally I'd remove the tests that are invalid expressions; but I don't know what's the usual practice. Ship it AFAIC!

This revision is now accepted and ready to land.Sat, Nov 13, 7:40 AM

Remove invalid testcases

Sorry, I meant remove the first four cases that are invalid C++, e.g.
auto{}.
Personally I'd keep the "valid but arguably misformatted" cases (which I
had suggested earlier but you just removed).
How that's clearer now.

Sorry my mistake, the 1st 4 I want to keep because they related to the unit tests for D113393: [c++2b] Implement P0849R8 auto(x)

foo(auto());   // expected-error {{initializer for functional-style cast to 'auto' is empty}}
foo(auto{});   // expected-error {{initializer for functional-style cast to 'auto' is empty}}
foo(auto({})); // expected-error {{cannot deduce actual type for 'auto' from parenthesized initializer list}}
foo(auto{{}}); // expected-error {{cannot deduce actual type for 'auto' from nested initializer list}}

Revert the last change but comment that the first 4 tests are from the unit tests of D113393 which are expected invalid (I wouldn't want the unit tests from formatting incorrectly)

Looks good. :)

Personally I'd remove the tests that are invalid expressions; but I don't know what's the usual practice. Ship it AFAIC!

It's nice if even invalid code gets formatted, because some of us have formatting on typing, and the then invalid code may become valid very quickly and it would be nice if the code does not jump around. :)

lichray added a comment.EditedSat, Nov 13, 11:45 AM

Thanks for the patch! It works for me, both for the auto{x} and new auto{x} cases. Some corner cases left:

void f() { T(&a)->n = 1; }
void f() { T{&a}->n = 1; }     // xxx
void g() { auto(&a)->n = 0; }
void h() { auto{&a}->n = 0; }  // xxx

The /// xxx cases are formatted as

void f() {
  T { &a } -> n = 1;
}

and

void h() {
  auto{ &a } -> n = 0;
}

But this may not worth a fix (for now).

void f() {
  T { &a } -> n = 1;
}

and

void h() {
  auto{ &a } -> n = 0;
}

But this may not worth a fix (for now).

Kibitzing: I don't think those cases matter to real code. But that spacing is so bizarre that it might be worth looking into anyway. Like, what does clang-format think auto{&n} even is? I can't think of any construct at all in C++ where we'd want to format it as T { anything or auto{ anything. I wonder if this means clang-format would misformat T{1, 2} as T { 1, 2 }! Basically the only reason to leave a space in the middle of identifier { is if you're concerned about cutesy macros like IF_CONSTEVAL {, but in those cases you should be looking for there to be a newline after the { anyway.

Thanks for the patch! It works for me, both for the auto{x} and new auto{x} cases. Some corner cases left:

void f() { T(&a)->n = 1; }
void f() { T{&a}->n = 1; }     // xxx
void g() { auto(&a)->n = 0; }
void h() { auto{&a}->n = 0; }  // xxx

The /// xxx cases are formatted as

void f() {
  T { &a } -> n = 1;
}

and

void h() {
  auto{ &a } -> n = 0;
}

But this may not worth a fix (for now).

I'm sure someone will look into it, but this should not be handled by this change.

minor comment and unit test change before committing

This revision was landed with ongoing or failed builds.Sun, Nov 14, 6:17 AM
This revision was automatically updated to reflect the committed changes.