Page MenuHomePhabricator

Clang-format: Braces Indent Style Whitesmith
Needs ReviewPublic

Authored by JVApen on Jan 3 2015, 1:27 AM.

Details

Reviewers
djasper
Summary

On request of Daniel,
the cleaned up patch.

For now, most of the stuff in the test works; and even running clang format on a sample code base gives some nice results.

Anyhow, the current issue still is that fixing the indent of the enumeration breaks other code.

I've also introduced the concept of a LevelGuard, since I got the feeling that doing the exact opposite of the increment of Line->Level bloats the code;
however is has not yet been aligned for the other styles. The position of those guards is currently one which is working; not yet the ideal one

Diff Detail

Event Timeline

JVApen updated this revision to Diff 17771.Jan 3 2015, 1:27 AM
JVApen retitled this revision from to Clang-format: Braces Indent Style Whitesmith.
JVApen updated this object.
JVApen edited the test plan for this revision. (Show Details)
JVApen added a reviewer: djasper.
JVApen added a subscriber: Unknown Object (MLST).
klimek added a comment.Jan 3 2015, 6:13 AM

Wouldn't it be simpler to not change the line level, and instead put an additional indent in for opening and closing braces?

djasper edited edge metadata.Jan 5 2015, 5:11 AM

I agree that that should lead to an way simpler implementation. It has the downside of spreading the brace handling stuff more outside of the UnwrappedLineParser, but that is probably ok in this case.

So, we should remove almost all the changes to the UnwrappedLineParser and instead modify UnwrappedLineFormatter::formatFirstToken(). JVApen, can you make those changes? Alternatively, I can also pick up and finish the patch.

JVApen added a comment.Jan 5 2015, 9:23 AM

I'm gonna assume you guys now the code base better then I do :p
By just indenting the braces, its gonna most likely be much cleaner code,
though then all code in UnwrappedLineParser will be the exact same for
Allman and Whitesmiths.
Any ideas how to make sure this will stay in sync in the future?

Anyhow, for now I'm gonna play a bit more with the code trying to do it the
'easy way' and get familiar with the code some more.
It won't hurt since I've noticed that I also need another feature to be
compliant with our current style sheet.
If it wouldn't work out I'll send a cry and otherwise a patch.

JVApen

JVApen updated this revision to Diff 18032.Jan 12 2015, 11:22 AM
JVApen edited edge metadata.

I've fixed the method to do the indentation, by adapting indent in ContinuationIndenter::getNewLineColumn()

As far as I understand it, the indent of the content of the enumeration is determined by the very last statement.
In a first attempt, I looked at the current token being an identifier; though it was not selective enough.
Now I look at the previous token, though this worked for the current tests, it gives incorrect indent for the second enum-value.

Further more this introduces an issue with the breaks, where:
switch (i)
..{
case A:
..{
..}
..break;
..}

​becomes:​

​switch (i)
..{
case A:
..{
..}
break;
..}

I'm really starting to get lost in this one function call :s

Is this issue dead? I'm looking to use the tool for a Whitesmiths formatted project and would be happy to finish off the work.

In case you still want to move forward with this, some more comments.

Why have you opted for doing this in getNewLineColumn instead of the UnwrappedLineFormatter::formatFirstToken() function I suggested? I think the latter would be way easier, but there might be cases I am not thinking of right now.

docs/ClangFormatStyleOptions.rst
260

indented?

lib/Format/ContinuationIndenter.h
270

No. I don't think it is necessary to add an extra value to the indent state for this and doing so would be costly in runtime.

timwoj added a subscriber: timwoj.Jun 11 2019, 12:19 PM

I updated this patch to remove all of the code from ContinuationIndenter and to use the newer BraceWrapping style option instead of setting each one individually in UnwrappedLineParser.

I still have the same issue with enums. Looking at the RootToken in UnwrappedLineFormatter::formatFirstToken when parsing an enum, the first pass through it has the entire line for the enum in it with all of the newlines stripped from my test case. It then doesn't ever return the braces as separate lines. If someone could give me a pointer as to how/why enums are tokenized differently from everything else, I can likely fix it. The test case I'm using looks like:

verifyFormat("enum X\n"
             "  {\n"
             "  Y = 0,\n"
             "  }\n",
             WhitesmithsBraceStyle);

I also have the issue with a break after a block inside of a case statement, as mentioned in https://reviews.llvm.org/D6833#107539. Other than those two, it's mostly working correctly. I removed the test for lambdas because I'm not entirely sure how that should even be formatted with Whitesmiths, but I can add it back in again.

Should I update this thread with the new diff against HEAD, or should I open a new one and close this one as dead?

insysion removed a subscriber: insysion.Jun 12 2019, 4:03 AM