Page MenuHomePhabricator

[clang-format] Align trailing comments if ColumnLimit is 0
ClosedPublic

Authored by pboettch on Aug 21 2017, 8:32 AM.

Details

Summary

ColumnLimit = 0 means no limit, so comment should always be aligned if requested. This was broken with

https://llvm.org/svn/llvm-project/cfe/trunk@304687

introduced via

https://reviews.llvm.org/D33830

and is included in 5.0.0-rc2. This commit fixes it and adds a unittest for this property.

Should go into clang-5.0 IMHO.

Diff Detail

Repository
rL LLVM

Event Timeline

pboettch created this revision.Aug 21 2017, 8:32 AM
djasper accepted this revision.Aug 21 2017, 8:42 AM
djasper added inline comments.
unittests/Format/FormatTestComments.cpp
2479 ↗(On Diff #111987)

s/Colums/ColumnLimit/

This revision is now accepted and ready to land.Aug 21 2017, 8:42 AM
krasimir requested changes to this revision.Aug 21 2017, 8:56 AM

@pboettch: could you point me to information about how backporting patches to 5.0.0 works? This patch doesn't apply cleanly at head.

lib/Format/WhitespaceManager.cpp
477 ↗(On Diff #111987)

If Style.ColumnLimit == 0, this subtraction overflows and gives a huge unsigned number. Could you please directly use UINT_MAX or something similar in that case?

This revision now requires changes to proceed.Aug 21 2017, 8:56 AM
pboettch added inline comments.Aug 21 2017, 9:15 AM
lib/Format/WhitespaceManager.cpp
477 ↗(On Diff #111987)

I saw that it will overflow when adding the == 0. My idea was to simply restore the behavior as it was before your modification.

It seems that ColumnLimit=0 is not well handled by clang-format. There are a lot of cases where ColumnLimit=10000 is not behaving the same way as =0.

pboettch updated this revision to Diff 111997.Aug 21 2017, 9:19 AM
pboettch edited edge metadata.

Integrated requests made by @djasper and @krasimir .

krasimir accepted this revision.Aug 21 2017, 9:52 AM

Thank you! I'll ask around if this can be merged into 5.0.0.

This revision is now accepted and ready to land.Aug 21 2017, 9:52 AM
djasper added inline comments.Aug 21 2017, 12:28 PM
lib/Format/WhitespaceManager.cpp
477 ↗(On Diff #111987)

That's very much intentional. ColumnLimit=10000 means (basically): Put as much as you can on one line. ColumnLimit=0 means: Try to stick with the original code's line breaks.

hans added a subscriber: hans.Aug 22 2017, 9:22 AM

Thank you! I'll ask around if this can be merged into 5.0.0.

Since it's fixing a regression, merging it sounds good to me. Let me know when it lands.

@pboettch, do you have commit access or should I commit this patch for you?

@krasimir No I don't have commit right. Feel free to commit this change. Thanks.

This revision was automatically updated to reflect the committed changes.