Page MenuHomePhabricator

[clang-format] Rework Whitesmiths mode to use line-level values in UnwrappedLineParser
ClosedPublic

Authored by timwoj on Jan 12 2021, 8:09 AM.

Details

Summary

This commit removes the old way of handling Whitesmiths mode in favor of just setting the
levels during parsing and letting the formatter handle it from there. It requires a bit of
special-casing during the parsing, but ends up a bit cleaner than before. It also removes
some of switch/case unit tests that don't really make much sense when dealing with
Whitesmiths.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 8:09 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay added a project: Restricted Project.
clang/unittests/Format/FormatTest.cpp
13707 ↗(On Diff #316099)

any reason why this is being removed?

HazardyKnusperkeks added inline comments.
clang/lib/Format/TokenAnalyzer.cpp
71

Unrelated change (although I think it's good one).

clang/lib/Format/UnwrappedLineParser.h
143 ↗(On Diff #316099)

A bool parameter for something like that is not very nice. An enum (class) is much nicer.

If one does not know the declaration addUnwrappedLine(false) seems a bit odd, addUnwrappedLine(LineLevel::Keep) looks better. (The naming may be changed.)

clang/unittests/Format/FormatTest.cpp
13528 ↗(On Diff #316099)

So until now it has formatted that always wrong?

timwoj added inline comments.Jan 13 2021, 4:31 PM
clang/lib/Format/TokenAnalyzer.cpp
71

Should I split that into a separate commit?

clang/unittests/Format/FormatTest.cpp
13528 ↗(On Diff #316099)

Hm, that's a good question. I'll have to think about that one. I guess for Whitesmiths and it's always wanting to indent braces, this should be fixed back to way that it was.

13707 ↗(On Diff #316099)

This is an artifact of there not being any sort of actual guide for Whitesmiths. I have it set now to always indent the case labels (and it should just ignore the IndentCaseLabels option entirely, like it does for IndentCaseBlocks). I can certainly fix that option to work again though.

clang/lib/Format/TokenAnalyzer.cpp
71

In my Opinion that would be the best.

MyDeveloperDay added inline comments.Jan 14 2021, 1:48 AM
clang/lib/Format/Format.cpp
819 ↗(On Diff #316099)

I guess its fine to define these defaults here but what if people turn them off in their config what will happen?

clang/unittests/Format/FormatTest.cpp
13707 ↗(On Diff #316099)

see the images from D93806: [clang-format] PR48569 clang-format fails to align case label with `switch` with Whitesmith Indentation, you are saying that anyone with bracewrappingsytle of Whitesmith cannot decide if they can or cannot indent case labels?

Could you show me a spec that show Whitesmiths is the other way around?

timwoj added inline comments.Jan 14 2021, 1:24 PM
clang/lib/Format/UnwrappedLineParser.h
143 ↗(On Diff #316099)

Noted, I'll make it an enum in the next update.

clang/unittests/Format/FormatTest.cpp
13707 ↗(On Diff #316099)

Part of the problem is that there isn't any sort of official spec for Whitesmiths that I've been able to find. I'm working on a project that uses it, and have been basically going off their layout as the proper way of doing things. Looking at those images, I agree that the option should continue to function.

MyDeveloperDay requested changes to this revision.Jan 17 2021, 3:17 AM
This revision now requires changes to proceed.Jan 17 2021, 3:17 AM
timwoj updated this revision to Diff 317765.Jan 19 2021, 8:55 PM

Updates from review:

  • Fixed handling of IndentCaseLabels
  • Fixed indentation of namespace braces
  • Replaced bool argument to addUnwrappedLine with an enum
timwoj updated this revision to Diff 317766.Jan 19 2021, 8:58 PM

Fixing diff to look at the right commit

Is there a test with indentation within a namespace? If not maybe you should add one.

clang/lib/Format/UnwrappedLineParser.cpp
613 ↗(On Diff #317766)

Maybe give !AddLevel && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths a name? You check it three times.

clang/lib/Format/UnwrappedLineParser.h
146 ↗(On Diff #317766)

I would prefer an enum class.

timwoj updated this revision to Diff 317924.Jan 20 2021, 10:11 AM

Add tests for Whitesmiths with all of the NamespaceIndentation options

MyDeveloperDay added inline comments.Jan 24 2021, 2:40 PM
clang/unittests/Format/FormatTest.cpp
13548 ↗(On Diff #317924)

you are removing the test with 1 namespace nesting

timwoj updated this revision to Diff 319170.Jan 25 2021, 5:04 PM

Restored lost single-nesting namespace test

MyDeveloperDay accepted this revision.Jan 26 2021, 10:17 AM

LGTM I think, it would be good to get input from other reviewers

clang/unittests/Format/FormatTest.cpp
13696 ↗(On Diff #319170)

whilst I don't really like changing the indentation I don't think there was anything that says it was correct before.

This revision is now accepted and ready to land.Jan 26 2021, 10:17 AM
curdeius requested changes to this revision.Jan 27 2021, 10:23 AM
curdeius added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2159 ↗(On Diff #319170)

Nit: please write consistently Whitesmiths with a capital W.

2163 ↗(On Diff #319170)

3*true? Please add comments a minima.

2259 ↗(On Diff #319170)

Nit: full stop at the end of the phrase.

2967 ↗(On Diff #319170)

Please move below to the place of use. Also, just naming it ClosesBlock is a bit misleading as it is only for Whitesmiths style.

613 ↗(On Diff #317766)

Agree on this.

clang/unittests/Format/FormatTest.cpp
13688 ↗(On Diff #319170)

Hmm, you don't test the same thing anymore...

13696 ↗(On Diff #319170)

That is really strange that case is indented differently than break. Is it intended? Is the style really like this?

This revision now requires changes to proceed.Jan 27 2021, 10:23 AM
curdeius added inline comments.Jan 27 2021, 12:30 PM
clang/unittests/Format/FormatTest.cpp
13696 ↗(On Diff #319170)

Forget it. I was thinking about something else.

timwoj added inline comments.Jan 27 2021, 3:36 PM
clang/lib/Format/UnwrappedLineParser.cpp
2967 ↗(On Diff #319170)

I agree with renaming it, but I'm not sure on moving it to where it's used. It relies on Line->MatchingOpeningBlockLineIndex which is reset shortly after this boolean value is calculated.

clang/unittests/Format/FormatTest.cpp
13688 ↗(On Diff #319170)

This change was intentional. According to the clang-format documentation, IndentCaseBlocks effectively just changes the brace breaking for the block after a case label. Given that it always breaks for Whitesmiths, IndentCaseBlocks can be effectively ignored for that style. I changed it to test the IndentCaseLabels instead because that tests indentation for the whole block.

timwoj updated this revision to Diff 319716.Jan 27 2021, 4:38 PM
  • Make LineLevel an enum class
  • Combine multiple duplicate checks into a named variable
  • Minor formatting fixes
curdeius accepted this revision.Jan 27 2021, 11:26 PM

LGTM. Thanks for working on this.

clang/lib/Format/UnwrappedLineParser.cpp
2967 ↗(On Diff #319170)

Right. I've overseen it. It's good now.

This revision is now accepted and ready to land.Jan 27 2021, 11:26 PM
timwoj marked 8 inline comments as done.Jan 28 2021, 10:44 AM

No problem, sorry it took a bit of back-and-forth there. Is there any way this can sneak into 12.0?

curdeius retitled this revision from Rework Whitesmiths mode to use line-level values in UnwrappedLineParser to [clang-format] Rework Whitesmiths mode to use line-level values in UnwrappedLineParser.Jan 29 2021, 5:22 AM

I don't think it can go into 12.x. But if you have a good reason and a related bug report you might add this bug ticket as a release blocker of https://llvm.org/PR48902.

BTW, would you mind updating clang/docs/ReleaseNotes.rst?

https://bugs.llvm.org/show_bug.cgi?id=48668 already exists as a bug report linked to this review. I'm not sure "the Zeek project would like to start using this" is a good enough reason to block an LLVM release though. I'm fairly certain there aren't a whole lot of people in the world using Whitesmiths. That said, there's already at least one clang-format bug in the release-blocker list.

I'll get the release notes updated shortly.

My advice leave it out of the release, the next release comes round pretty quickly. This gives us 6 months of people who use the snapshots to iron out any issues.

timwoj updated this revision to Diff 322477.Feb 9 2021, 12:57 PM

Added release note entry

HazardyKnusperkeks requested changes to this revision.Feb 10 2021, 1:07 AM

Have you rebased your change? It seems that your release notes are still for LLVM 12.

This revision now requires changes to proceed.Feb 10 2021, 1:07 AM
timwoj updated this revision to Diff 322689.Feb 10 2021, 7:54 AM

Rebased on main

This revision is now accepted and ready to land.Feb 10 2021, 12:53 PM

Do you need someone to push this?

Do you need someone to push this?

Yes I do. I don't have committer access.

Do you need someone to push this?

Yes I do. I don't have committer access.

Please state the name and mail for the commit.

timwoj added a comment.Mar 1 2021, 4:54 PM

Do you need someone to push this?

Yes I do. I don't have committer access.

Please state the name and mail for the commit.

Tim Wojtulewicz
timwoj@gmail.com

Could you please rebase your patch an reupload the diff, it doesn't apply anymore. (And doing it by hand is too much work for me.)

timwoj updated this revision to Diff 328308.Mar 4 2021, 2:17 PM

Rebase onto master again

Rebase onto master again

master or main? Please rebase on main. That is where the pushes land. (And as far as I can see master hasn't been updated in quite some time.

timwoj added a comment.Mar 4 2021, 9:32 PM

Rebase onto master again

master or main? Please rebase on main. That is where the pushes land. (And as far as I can see master hasn't been updated in quite some time.

Yes, main, sorry. It was late in my work day and I was fried already.

Rebase onto master again

master or main? Please rebase on main. That is where the pushes land. (And as far as I can see master hasn't been updated in quite some time.

Yes, main, sorry. It was late in my work day and I was fried already.

No problem.