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

Unit TestsFailed

TimeTest
40 msx64 debian > Flang.Semantics::resolve102.f90
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/test_errors.sh /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/resolve102.f90 /mnt/disks/ssd0/agent/llvm-project/build/tools/flang/test/Semantics/Output/resolve102.f90.tmp /mnt/disks/ssd0/agent/llvm-project/build/bin/f18 -intrinsic-module-directory /mnt/disks/ssd0/agent/llvm-project/build/tools/flang/include/flang
140 msx64 windows > lld.MachO::rename.s
Script: -- : 'RUN: at line 2'; rm -fr C:\ws\w16n2-1\llvm-project\premerge-checks\build\tools\lld\test\MachO\Output\rename.s.tmp

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
2241

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
2241

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.