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
|71 ↗||(On Diff #316099)|
Unrelated change (although I think it's good one).
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.)
So until now it has formatted that always wrong?
|71 ↗||(On Diff #316099)|
Should I split that into a separate commit?
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.
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.
|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?
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?
Noted, I'll make it an enum in the next update.
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.
Is there a test with indentation within a namespace? If not maybe you should add one.
Maybe give !AddLevel && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths a name? You check it three times.
I would prefer an enum class.
Agree on this.
Nit: please write consistently Whitesmiths with a capital W.
3*true? Please add comments a minima.
Nit: full stop at the end of the phrase.
Please move below to the place of use. Also, just naming it ClosesBlock is a bit misleading as it is only for Whitesmiths style.
Hmm, you don't test the same thing anymore...
That is really strange that case is indented differently than break. Is it intended? Is the style really like this?
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.
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.
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.
A crash (https://bugs.llvm.org/show_bug.cgi?id=50663) in 12.0.1 is fixed by your changes here when merged to the 12 branch, I'm not sure if this needs to be backported to 12 I guess it might depend on if we think it's critical enough or if there will more 12 release candidates before 13 @timwoj any thoughts?
It should apply cleanly to 12.0 as far as I know, since if I remember right I had asked if it could be merged before 12.0 was released. I don't have a whole lot of free cycles at the moment to help with it if it needs a lot of changes though. My vote is yes to backport it, if it counts for anything.
I think we've missed the deadline for 12.0.1 which I believe was last friday. I did a practice merge and it didn't quite go in cleanly to 12.0.X branch
Normally we would mark the bug in the bug tracker for the next release, this is what I did for another bug and @tstellar I think picked it up and merge it for us.
I'm not sure if we are doing a 12.0.2 release but I've added that as the "Blocks" tag in the bug tracker, one of us can merge it if Tom wants us to.
To be honest looking at the timelines
I'd probably say can it just wait for 13.0