This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Rewrite how indent is reduced for compacted namespaces
ClosedPublic

Authored by rymiel on Feb 17 2023, 1:14 PM.

Details

Summary

The previous version set the indentation directly using IndentForLevel,
however, this has a few caveats, namely:

  • IndentForLevel applies to all scopes of the entire program being formatted, but this indentation should only be adjusted for scopes of namespaces.
  • The method it used only set the correct indent amount if one wasn't already set for a given level, meaning it didn't work correctly if anything with indentation preceded a namespace keyword. This includes preprocessing directives if using IndentPPDirectives.

This patch instead reduces the Level of all lines within namespaces
which are compacted by CompactNamespaces. This means they will get
correct indentation using the normal process.

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

Diff Detail

Event Timeline

rymiel created this revision.Feb 17 2023, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 1:14 PM
rymiel requested review of this revision.Feb 17 2023, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2023, 1:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rymiel edited the summary of this revision. (Show Details)Feb 17 2023, 1:15 PM

Note: this patch also makes LevelIndentTracker::skipLine obsolete. I suppose I could clean that up in a following patch

There is no PP stuff in the new tests.

And I see the style was bogus before, but shouldn't all variables start with a capital letter? Please fix that in the code you touch.

Note: this patch also makes LevelIndentTracker::skipLine obsolete. I suppose I could clean that up in a following patch

I'd delete it in this patch.

clang/lib/Format/UnwrappedLineFormatter.cpp
369
371–373
377
381
383

outdentBy?

385
387

Did git-clang-format miss this?

And I see the style was bogus before, but shouldn't all variables start with a capital letter? Please fix that in the code you touch.

+1.

rymiel updated this revision to Diff 499328.Feb 21 2023, 5:07 PM
rymiel marked 7 inline comments as done.

Apply suggestions and add extra test

clang/lib/Format/UnwrappedLineFormatter.cpp
387

Nope, I forgot to amend the change!

rymiel updated this revision to Diff 499330.Feb 21 2023, 5:10 PM

Inline and remove LevelIndentTracker::skipLine

owenpan accepted this revision.Feb 21 2023, 5:23 PM
This revision is now accepted and ready to land.Feb 21 2023, 5:23 PM
MyDeveloperDay accepted this revision.Feb 22 2023, 12:32 PM
This revision was landed with ongoing or failed builds.Feb 25 2023, 2:18 AM
This revision was automatically updated to reflect the committed changes.