Page MenuHomePhabricator

[clang-format] Respect spaces in line comment section without an active column limit
ClosedPublic

Authored by HazardyKnusperkeks on Feb 17 2021, 1:19 PM.

Details

Summary

Before line comments were not touched at all with ColumnLimit == 0, so this is a real change. But at least for me it was unexpected that the comments were not fixed with ColumnLimit == 0.

Diff Detail

Event Timeline

HazardyKnusperkeks requested review of this revision.Feb 17 2021, 1:19 PM
HazardyKnusperkeks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2021, 1:19 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

It seems to be a bug indeed.
So, basically all the changes are just adding a bunch of conditions on ColumnLimit != 0 which is a special value meaning "no limit", right? Or have I missed something?
Thining out loud... wouldn't it be easier to change ColumnLimit from 0 to numeric_limits<unsigned>::max() just before these transformations?

clang/lib/Format/ContinuationIndenter.cpp
2243

Is this condition on ColumnLimit (and other below) working ok when ColumnLimit == 0? Do your tests cover it?

HazardyKnusperkeks planned changes to this revision.Feb 18 2021, 11:17 AM
HazardyKnusperkeks added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
2243

I must admit, that I don't know for sure.
Line comments should be checked, and a string literal on one line is also covered (that's how I found that I have to disable the inner loop). I should add a new test case with multiline comments and multiline string literals.

But I think your proposal of using ::max() is sound and should be used.

  • Work with ::max instead of many checks for == 0.
  • Additional Testcase
curdeius accepted this revision.Feb 28 2021, 1:43 AM

LGTM.

This revision is now accepted and ready to land.Feb 28 2021, 1:43 AM
This revision was landed with ongoing or failed builds.Mar 1 2021, 12:28 PM
This revision was automatically updated to reflect the committed changes.