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.
Details
- Reviewers
MyDeveloperDay curdeius HazardyKnusperkeks
Diff Detail
- Repository
- rG LLVM Github Monorepo
Time | Test | |
---|---|---|
80 ms | x64 windows > Clang.Driver::undefined-libs.cpp Script:
--
: 'RUN: at line 5'; not c:\ws\w64\llvm-project\premerge-checks\build\bin\clang.exe --driver-mode=g++ -target=i386-unknown-linux -stdlib=nostdlib C:\ws\w64\llvm-project\premerge-checks\clang\test\Driver\undefined-libs.cpp 2>&1 | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe --check-prefix=STDLIB C:\ws\w64\llvm-project\premerge-checks\clang\test\Driver\undefined-libs.cpp
|
Event Timeline
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
14885 | any reason why this is being removed? |
clang/lib/Format/TokenAnalyzer.cpp | ||
---|---|---|
71 ↗ | (On Diff #316099) | Unrelated change (although I think it's good one). |
clang/lib/Format/UnwrappedLineParser.h | ||
144–149 | 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 | ||
14706 | So until now it has formatted that always wrong? |
clang/lib/Format/TokenAnalyzer.cpp | ||
---|---|---|
71 ↗ | (On Diff #316099) | Should I split that into a separate commit? |
clang/unittests/Format/FormatTest.cpp | ||
14706 | 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. | |
14885 | 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 ↗ | (On Diff #316099) | In my Opinion that would be the best. |
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 | ||
14885 | 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? |
clang/lib/Format/UnwrappedLineParser.h | ||
---|---|---|
144–149 | Noted, I'll make it an enum in the next update. | |
clang/unittests/Format/FormatTest.cpp | ||
14885 | 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. |
Updates from review:
- Fixed handling of IndentCaseLabels
- Fixed indentation of namespace braces
- Replaced bool argument to addUnwrappedLine with an enum
Is there a test with indentation within a namespace? If not maybe you should add one.
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
614 | Maybe give !AddLevel && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths a name? You check it three times. | |
clang/lib/Format/UnwrappedLineParser.h | ||
147 | I would prefer an enum class. |
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
14726 | you are removing the test with 1 namespace nesting |
LGTM I think, it would be good to get input from other reviewers
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
14874 | whilst I don't really like changing the indentation I don't think there was anything that says it was correct before. |
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
614 | Agree on this. | |
2148 | Nit: please write consistently Whitesmiths with a capital W. | |
2152 | 3*true? Please add comments a minima. | |
2254 | Nit: full stop at the end of the phrase. | |
2962 | Please move below to the place of use. Also, just naming it ClosesBlock is a bit misleading as it is only for Whitesmiths style. | |
clang/unittests/Format/FormatTest.cpp | ||
14866 | Hmm, you don't test the same thing anymore... | |
14874 | That is really strange that case is indented differently than break. Is it intended? Is the style really like this? |
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
14874 | Forget it. I was thinking about something else. |
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
2962 | 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 | ||
14866 | 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. |
- Make LineLevel an enum class
- Combine multiple duplicate checks into a named variable
- Minor formatting fixes
LGTM. Thanks for working on this.
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
2962 | Right. I've overseen it. It's good now. |
No problem, sorry it took a bit of back-and-forth there. Is there any way this can sneak into 12.0?
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.
Have you rebased your change? It seems that your release notes are still for LLVM 12.