This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix overlapping whitespace replacements before PPDirective
ClosedPublic

Authored by owenpan on Jun 1 2023, 6:02 PM.

Details

Summary

If the first token of an annotated line already has a computed Newlines, reuse it to avoid potential overlapping whitespace replacements before preprocessor branching directives.

Fixes https://github.com/llvm/llvm-project/issues/62892.

Diff Detail

Event Timeline

owenpan created this revision.Jun 1 2023, 6:02 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJun 1 2023, 6:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
owenpan requested review of this revision.Jun 1 2023, 6:02 PM
owenpan added inline comments.Jun 1 2023, 9:38 PM
clang/lib/Format/UnwrappedLineFormatter.cpp
1425

I will remove this parameter as it's Line.First.

owenpan updated this revision to Diff 527765.Jun 2 2023, 12:12 AM

Removed the RootToken parameter.

MyDeveloperDay accepted this revision.Jun 2 2023, 2:22 AM
This revision is now accepted and ready to land.Jun 2 2023, 2:22 AM
This revision was landed with ongoing or failed builds.Jun 3 2023, 12:33 PM
This revision was automatically updated to reflect the committed changes.

This patch seems to introduce some significant regressions to formatting. I've filed https://github.com/llvm/llvm-project/issues/63170 with some candidate code. I'll try to reduce the case as I can, but I'm guessing something is incorrect w/ how the indentation level is tracked, since we see includes being incorrectly indented and lots of lines w/ whitespace.

I'd also suggest reverting this until it can be fixed

This patch seems to introduce some significant regressions to formatting. I've filed https://github.com/llvm/llvm-project/issues/63170 with some candidate code. I'll try to reduce the case as I can, but I'm guessing something is incorrect w/ how the indentation level is tracked, since we see includes being incorrectly indented and lots of lines w/ whitespace.

https://github.com/llvm/llvm-project/issues/63170#issuecomment-1581331587

@leonardchan I'm not sure this patch should be reverted just yet. I can't reproduce the regression, and this patch does fix a previous regression.

owenpan updated this revision to Diff 531538.Jun 14 2023, 3:30 PM
owenpan edited the summary of this revision. (Show Details)
owenpan added a reviewer: paulkirth.
owenpan removed a subscriber: paulkirth.

Add a member Newlines to struct FormatToken and compute it only once in UnwrappedLineFormatter::formatFirstToken().

owenpan reopened this revision.Jun 14 2023, 3:30 PM
This revision is now accepted and ready to land.Jun 14 2023, 3:30 PM
owenpan requested review of this revision.Jun 14 2023, 3:32 PM
This revision is now accepted and ready to land.Jun 15 2023, 12:51 PM
MyDeveloperDay accepted this revision.Jun 16 2023, 3:35 PM
This revision was landed with ongoing or failed builds.Jun 16 2023, 5:00 PM
This revision was automatically updated to reflect the committed changes.

Rebased and changed a unit test to use verifyNoChange before landing.