This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] respect AfterEnum for enums
ClosedPublic

Authored by m1cha on Jul 20 2021, 1:30 AM.

Details

Summary

There is some similar looking code in TokenAnnotator.cpp but given that I've
never worked on clang-format before I don't know what the purpose of that code
is and how it's related to UnwrappedLineParser.cpp.

Either way, it fixes clang-format with BraceWrapping.AfterEnum=true and
AllowShortEnumsOnASingleLine=false to behave like the documentation says.

Before this patch:

enum
{
  A,
  B
} myEnum;

After this patch:

enum {
  A,
  B
} myEnum;

According to the unittests which I had to modify this would change the LLVM
style. Please evaluate if you want to change the defaults or if you consider
the current style a bug.

Diff Detail

Event Timeline

m1cha requested review of this revision.Jul 20 2021, 1:30 AM
m1cha created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2021, 1:30 AM
m1cha added a comment.Jul 20 2021, 1:35 AM

I just noticed that phabricator strips the commit message so here it is:

There is some similar looking code in TokenAnnotator.cpp but given that I've
never worked on clang-format before I don't know what the purpose of that code
is and how it's related to UnwrappedLineParser.cpp.

Either way, it fixes clang-format with BraceWrapping.AfterEnum=true and
AllowShortEnumsOnASingleLine=false to behave like the documentation says.

Before this patch:

enum
{
  A,
  B
} myEnum;

After this patch:

enum {
  A,
  B
} myEnum;

According to the unittests which I had to modify this would change the LLVM
style. Please evaluate if you want to change the defaults or if you consider
the current style a bug.

This feels like there is some overlap with D93938: [clang-format] Fixed AfterEnum handling

Yeah that may be, but it is stale for over a month, with I think open questions. So if this here fixes at least one bug with no obvious new ones, I think we should go for it.

Regarding the patch: Please upload it with the context available. https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

m1cha updated this revision to Diff 360776.Jul 22 2021, 5:31 AM
m1cha edited the summary of this revision. (Show Details)
  • rebased to main
  • fixed the test IndentAccessModifiers

Looks good, could you also add a note in the relasenotes.rst?

Looks good, could you also add a note in the relasenotes.rst?

Sure

Can I do something about the failing unittests? I don't know how they are related to the changes I made.

Looks good, could you also add a note in the relasenotes.rst?

Sure

Can I do something about the failing unittests? I don't know how they are related to the changes I made.

They (most likely) aren't. That's something which set me off at the beginning too. If the Format Tests pass on your machine most probably everything is fine for you and for clang-format.
When you update the patch with release notes you can rebase on current main, maybe it is already fixed, maybe not.

m1cha updated this revision to Diff 361493.Jul 25 2021, 12:49 AM

I've added a note to ReleaseNotes.rst

This revision is now accepted and ready to land.Jul 25 2021, 12:52 AM

No one objects, you can push it. Or if you don't have commit access (and don't want to, or don't want to wait for it) we can push it, then please state name and email for the commit.

m1cha added a comment.Jul 29 2021, 1:02 PM

I don't have commit access. How does that even work? The documentation is very scarce about this but for security reasons I'd expect there to be a very limited set of people with commit access

Here's my author info:
Michael Zimmermann <sigmaepsilon92@gmail.com>

lunasorcery added a subscriber: lunasorcery.EditedJul 29 2021, 1:14 PM

Note also there's significant overlap with the now-committed D99840, though that's missing the change inside ShouldBreakBeforeBrace().
In retrospect I'm a little confused as to why D99840 appears to work without it.

I don't have commit access. How does that even work? The documentation is very scarce about this but for security reasons I'd expect there to be a very limited set of people with commit access

Here's my author info:
Michael Zimmermann <sigmaepsilon92@gmail.com>

https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access I don't think it is limited, I'm also just some guy which applied for it some 7 months ago. Since the next release is far away, you can apply for the access and commit it yourself. I did only wait one day. ;)

Note also there's significant overlap with the now-committed D99840, though that's missing the change inside ShouldBreakBeforeBrace().
In retrospect I'm a little confused as to why D99840 appears to work without it.

I thought it looked familiar, but did not find what it was. Does this change needs to be rebased before it can be applied?

m1cha added a comment.Jul 31 2021, 8:38 AM

Well it's pretty much exactly the same except for the change in ShouldBreakBeforeBrace.
Somebody with more knowledge about clang-format than me should try to figure out why it's not needed.

If it's actually not needed than this change does not need to be merged.

Copying over the comment I left on D99840 explaining it - my existing change only works because it gets bailed out by further code in TokenAnnotator.cpp.
I think your change still is valuable since it's more correct.

Looking at this again I think the change is flawed. The InitialToken is taken *after* the enum token, and additionally the invocation of ShouldBreakBeforeBrace is falling through to the return false; case every time. As such the call to addUnwrappedLine() is _always_ skipped. This removes the newline for both the Attach and Break styles - but the Break style is then fixed up by some code in TokenAnnotator.cpp that I don't fully understand, which adds the newline back in.

So [D99840] _works_, but not quite for the right reasons.

D106349 appears to implement this in a slightly more correct manner, taking the InitialToken _before_ we've advanced past the enum token, and handling the enum case inside ShouldBreakBeforeBrace.

curdeius accepted this revision.Jan 3 2022, 2:58 AM

LGTM. If nobody objects, I'll land this today.

no objection from me, we need to clean up the non landed accepted reviews or it wastes our time in the first place.

This revision was landed with ongoing or failed builds.Jan 3 2022, 11:01 AM
This revision was automatically updated to reflect the committed changes.