Page MenuHomePhabricator

[clang-format] Fix the bug that joins template closer and > or >>
ClosedPublic

Authored by owenpan on Aug 15 2019, 10:02 PM.

Diff Detail

Event Timeline

owenpan created this revision.Aug 15 2019, 10:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2019, 10:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
This revision is now accepted and ready to land.Aug 16 2019, 10:58 AM

Drive-by observation: My experiments with https://zed0.co.uk/clang-format-configurator/ show that there is a similar bug where clang-format accidentally produces >= via reformatting of template<enable_if_t<Foo, int> =0>, pi<int> =3;, and so on. Is it possible to fix that bug as part of this patch, and/or would you be interested in patching that bug as a follow-up to this one?

Drive-by observation: My experiments with https://zed0.co.uk/clang-format-configurator/ show that there is a similar bug where clang-format accidentally produces >= via reformatting of template<enable_if_t<Foo, int> =0>, pi<int> =3;, and so on. Is it possible to fix that bug as part of this patch, and/or would you be interested in patching that bug as a follow-up to this one?

Do you have SpaceBeforeAssignmentOperators off? I am more than happy to fix it as a follow-up, but the current behavior of SpaceBeforeAssignmentOperators is:
true:

int a = 5;
a += 42;

false:

int a= 5;
a+= 42;

It's weird that there is a space after the assignment operators regardless.

The documentation should be fixed.

This revision was automatically updated to reflect the committed changes.

Do you have SpaceBeforeAssignmentOperators off?

Yes; I mean, that's what I tested in order to produce the buggy behavior. I don't believe anyone should be using that option in production, though, and would totally support a patch to remove it completely because it can't possibly be doing anyone any good. It feels like someone meant to implement SpacesAroundAssignmentOperators but then got the name wrong, and then someone fixed the behavior to match the name, and at this point it's just completely broken. :)

The documentation should be fixed.

Also agreed.

Are there any other clang-format options that might lead to a lack-of-space before >, >=, ==, or =? I brought up enable_if_t<Foo, int>=0 because that specifically is a construction I've run into in my own code, even though I've never tried to clang-format it, let alone have it formatted incorrectly.

(Clang-format probably does not promise to preserve the AST of pathological input like i++ ++; or C++03-specific input such as A<B<int> > or A< ::B>.)

owenpan added a comment.EditedAug 16 2019, 10:48 PM

Are there any other clang-format options that might lead to a lack-of-space before >, >=, ==, or =? I brought up enable_if_t<Foo, int>=0 because that specifically is a construction I've run into in my own code, even though I've never tried to clang-format it, let alone have it formatted incorrectly.

Any token that starts with >, e.g., >, >>, >=, and >>=, are already taken care of by this patch. For tokens starting with =, only the assignment operator = has a problem and it only occurs when SpaceBeforeAssignmentOperators is set to false.

I can insert a space between a template closer > and an assignment operator = despite the value of SpaceBeforeAssignmentOperators, but the formatted code may be inconsistent...