Replaced the table form with dubious comments with the actual variables
Details
Diff Detail
Event Timeline
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
1088–1093 | something here looks abit odd? there is too much repetition around you option, I think you doing something at the wrong level. |
The comments were only for testing, I'll remove them.
The tests had to change because the behavior has changed slightly.
In practice it should be the same because LLVMStyle.IndentExternBlock default is set to false, but previously the BraceWrapping.AfterExternBlock = true code would indent as well, and now the behavior of BraceWrapping.AfterExternBlock only effects the brace wrapping.
As for the release notes, which file should I edit for that, and also which version will this even end up in? probably 11 right, because 10 is in RC status right now?
Which other files? I made a new commit and did the full context diff, now sure why it's only showing the documentation update.
I've never committed to Clang before, I don't know this process exactly.
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
1088–1093 | I agree that the parseBlock function is doing too much, but it's written that way to begin with. The parseBlock function takes 3 parameters and has defaults for two of them, I just changed the value for those defaults on the IndentExternBlock == false versions to default to not indenting; that way the AfterExternBlock option only handles bracewrapping extern blocks, without indenting as well. |
Rebased on Master again, recompiling and re-running all the tests.
I'll update this comment when it passes, or create a new diff if it doesn't but nothing had to be changed so it'll probably work.
@krasimir I noticed that you've been active recently, can you review my patch?
Not sure if tagging is considered rude, I figure that @MyDeveloperDay's notification fell off your radar.
Edit: The tests still pass.
@MarcusJohnson91 To get a review past its better that you mark the comments as done so the reviewers know when the comments have been addressed and not missed. (this is important as the number of comments grows)
Developers need to explain why they haven't changed something the reviewer has commented on, in some form or another, that means either an explanation of why they are not doing it, or they do it.. but leaving it ignored makes the reviewer feel the author hasn't got round to looking at it yet or missed it and so it isn't ready for re-review.
As for me, this is not off my radar,I get busy at work which means my time is reduced, but I visit the side daily and review where we are. I've been back to this review 4 or 5 times to this one alone (normally on every ping) but the comments have not been addressed, so I'm afraid I'm unlikely to now go around as say, "Yeah its fine, put it in" without anything changing or being explained why. I probably should have explained that earlier (which is my bad).
Every reviewer has a different bar, for me its people changing tests.. In the review the tests changed and I'm thinking sorry but I can't trust its not breaking existing behavior. (I go by the Beyonce rule of testing, and no one should be changing Beyonce as surely she's perfect already!)
I've met this in my professional life where developers modify tests until their code works, and while you may not be doing that here, I have a personal aversion to them being changed having been burnt a few times. I personally don't care how many tests there are, the more the merrier is my view.
I also don't like tests which test 2 things at the same time. For me line #2463 is it. Its super subtle and probably 100% OK, but I don't understand why you added the /* comment */, (by all means add that as another test), but I don't like changing the existing tests unless I absolutely have to.
My advice is address those comments, mark them done, but by all means canvas opinion from others if you think that's unfair. Its just my opinion, I'm not the only reviewer able to give an Accept.
clang/lib/Format/UnwrappedLineParser.cpp | ||
---|---|---|
1088–1093 | I still feel this blocks of 4 if can be written better, its making my eyes hurt, every if calls parseBlock() with either true.true, of true,false I feel it could be something like if (Style.BraceWrapping.AfterExternBlock){ addUnwrappedLine() } parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/!Style.BraceWrapping.AfterExternBlock); |
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
2444–2446 | I know why you change these varaibles becuase when your developing its hard to see which test is failing because verifyFormat doesn't report the correct file and line number but I like the use of foo because its 1 character more than your indent width and if there is going to be an out by one error you might hide it with a longer name |
Not sure if tagging is considered rude, I figure that @MyDeveloperDay's notification fell off your radar.
Definitely not rude from my perspective...perfectly happy to come and look and be tagged to get my attention. If I'm ignoring its, its only becuase I'm busy but will get to it asap.
Implemented @MyDeveloperDay's suggestion to simplify the if/else statements.
Removed all the test changes except one:
That's because the BraceWrapping.AfterExternBlock option has been changed so that it no longer indents and wraps the brace; now it just wraps the brace, and the old version of the test expected indenting.
Also Rebased on Master, and made sure all the tests passed, including the new tests I wrote for this feature.
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
31 | Removed the comment, it was just a note to myself that fell through the cracks | |
2440 | These test comments were adds solely so I could see which part of the tests we're failing, because there's some repeats and it got confusing. Clang's tests would print the line which included the text, it was basically printf debugging without printf. There is now just one minor change to the existing tests. |
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
2440 | actually I have a change in https://reviews.llvm.org/D69764#change-XlRuRVhsnCkV which helps address this, but it feels too much to roll out. Instead of using verifyFormat we use a macro VERIFYFORMAT(), this passes the LINE and FILE across into the gtest code which then means when these verifyFormat() fail it actually tells you which line its failing on! | |
2474 | my assumption is this test is using Style.IndentExternBlock =false correct? This suggests the default was previously true not false if (Style.BraceWrapping.AfterExternBlock) { if (Style.BraceWrapping.AfterExternBlock) { addUnwrappedLine(); parseBlock(/*MustBeDeclaration=*/true); } else { parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/false); } This one test change alone, makes me see that it might be incorrect to set the default Indent to false when AfterExternBlock is true. (parseBlock default parameter for AddLevel is true) shouldn't the default value of Style.IndentExternBlock = Style.BraceWrapping.AfterExternBlock? (which is 100% why I don't like seeing tests changed).. this was buried in your last changes to the tests and I didn't see it.) So now we need to go back and take a look through the clang sources and see what its doing by default, and tests what other default styles are doing prior to this change and if they indent then I think by default we need to indent. If we run clang-format on the following code, we see an issues Whats great about this Fix (which is why it needs to go in) this test file despite being part of LLVM its not actually formatted with the LLVM style ;-) i.e. it will come out as extern "C" { extern "C" void f(int); } extern "C++" { extern "C++" int &g(int); float &g(); } instead of extern "C" { extern "C" void f(int); } extern "C++" { extern "C++" int& g(int); float& g(); } Thats because they don't want a break after the "C" and before the { but they do what the indent. Conversely there is code in This code IS more formatted than the the previous one (not 100% correct but close enough) and so having the default to true would cause unnecessary churn here. Then of course there must be other projects that use BreakAfterExtern=true and they would get the indentation and want to keep it, so the setting of the default value of IndentExternBlock needs to be dynamic if possible, don't you think. To be honest your change should support actually allowing this some of these files to now support keeping its format correclty, which in my view is great! but breaking the default styles is always hard, because we get complaints that different versions of clang-format can cause huge waves of changes and I've seen unless teams are all on the same version then the format can flipflop between versions. | |
2482 | nit: these are a little easier to read when formatted as above verifyFormat("extern \"C\" {\n" " int foo();\n" "}", Style); |
clang/unittests/Format/FormatTest.cpp | ||
---|---|---|
2474 | Yes, this test is defaulting to IndentExternBlock = false, because when I was initially looking at examples of C code for Google's style, Micorosft, WebKit, LLVM, etc (all the built in styles) it appeared that LLVM does not indent their extern blocks. As for linkage-spec.cpp, that file indents extern blocks, and donotoptimize_assembly_test.cc doesn't as you pointed out. As for why I chose to default LLVM to indenting, I don't remember which files specifically gave me the impression that LLVM indents. As for changing the default behavior, I agree It sucks, but I'm not sure how we could have this feature without changing the behavior. Maybe I should just remove the default for LLVM, or maybe I shouldn't default to anything for any style period? | |
2482 | Yeah, it really tripped me up reading these string literals when they were cut up into multiple pieces but without commas between them, so that's why I wrote these this way. I'm not against breaking them up, it was just initially confusing to me. |
Could the default be Style.IndentExternBlock = Style.BraceWrapping.AfterExternBlock
Then it would match the prior behaviour off AddLevel being true when AfterExternBlock is true
extern "C" { int a; } extern "C" { int a; }
I have no concerns with this patch other than the consideration of the defaults.
My concern is whatever our choice we will generate churn on existing code where the .clang-format file has AfterExternBlock = true (or its true in inbuilt BasedOnStyle type)
https://github.com/search?q=%22AfterExternBlock%3A+true%22&type=Code
This is also true for all Mozilla and Allman styles, I'd be happy to give this a LGTM, but I feel like I'll end up saying "told you so on the choice of defaults" when the complaints come in, unless we are able to say
Style.IndentExternBlock = Style.BraceWrapping.AfterExternBlock
The problem is really that we need to detect the existence of IndentExternBlock missing from the config and a boolean isn't the best choice for this because we have 3 states (NotSpecified,Indent,DontIndent)
I'm uncomfortable with the decision we've made that says IndentExternBlock is by default false when previously if we were putting a break we were following the normal indenting rules of all other control blocks
parseBlock(/*MustBeDeclaration=*/true,
/*AddLevel=*/Style.IndentExternBlock);
If we can't solve that, I'm not sure I'm happy to Accept, but code wise I'm fine with everything else. I'm not sure if I'm overly worrying.
There is some code in the void mapping(IO &IO, FormatStyle &Style) that could show you a way to do this..
but I'm also constantly surprised by how many of the enumeration cases started out as booleans only later to have to be converted to enums. The more I think about this the more I think the problem can probably be dealt with better by making it an enumeration. (even if you support true and false to mean "indent" and "don't indent"
template <> struct ScalarEnumerationTraits<FormatStyle::BracketAlignmentStyle> { static void enumeration(IO &IO, FormatStyle::BracketAlignmentStyle &Value) { IO.enumCase(Value, "Align", FormatStyle::BAS_Align); IO.enumCase(Value, "DontAlign", FormatStyle::BAS_DontAlign); IO.enumCase(Value, "AlwaysBreak", FormatStyle::BAS_AlwaysBreak); // For backward compatibility. IO.enumCase(Value, "true", FormatStyle::BAS_Align); IO.enumCase(Value, "false", FormatStyle::BAS_DontAlign); } };
I agree that changing formatting randomly isn't a good idea, and I think converting AfterExternBlock to an enum is the way to go, but I'm just not sure on how it should be implemented.
Ok, I've got an idea to deprecate the AfterExternBlock option and map it to a new option, I'm gonna start implementing it right now.
Ok, I've got an idea to deprecate the AfterExternBlock option and map it to a new option
Really? can't you just model IndentExternBlock as an enum? then say
bool shouldIndent = Style.IndentExternBlock==Indented || ( AfterExternBlock && Style.IndentExternBlock==NotSpecified) parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/shouldIndent);
You can even keep "true" and "false" the the enumeration marshaller and you don't even have to change the documentation.
but I'm also constantly surprised by how many of the enumeration cases started out as booleans only later to have to be converted to enums. The more I think about this the more I think the problem can probably be dealt with better by making it an enumeration. (even if you support true and false to mean "indent" and "don't indent"
I FULLY support all new options being required to be enums from now on, bools cause a whole lotta trouble when they have to be changed.
I've rewritten my patch, it works when manually testing it, now I'm just working on the automated tests.
A brand new patch should be up by either tonight or tomorrow.
You need to regenerate the ClangFormatStyleOption.rst file using docs/tools/dump_format_style.py
clang/include/clang/Format/Format.h | ||
---|---|---|
965 | I'm a little confused here by the use of this enum, can't it be removed now? | |
clang/lib/Format/UnwrappedLineParser.cpp | ||
1087–1088 | can't you just use? parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.BraceWrapping.AfterExternBlock); | |
1094–1102 | parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.IndentExternBlock == FormatStyle::IEBS_Indent) |
LGTM just drop the Noindent it will encourage inconsistency people can read the manual (plus you don't check it in the tests)
clang/lib/Format/Format.cpp | ||
---|---|---|
212 | I don't think that's necessary |
Removed the lowercase Noindent case, that was a last minute addition I thought might make it a tad easier to work with, but you're right I didn't even test it, and honestly adding that complexity is just pointless at best.
Removed.
In my mind you have two choices
- require commit access
- get someone else to land it
If you think you'd like to continue to be involved and maybe help out from time to time (especially with reviews which is where we REALLY lack people) then I would recommend 1)
but I'm happy to do 2) for you if not, but you have to be prepared to support the patch a little afterwards. (no fire and forget please ;-) )
Please clang-format the patch, I'm also getting a crash when running the tests, please make sure they pass.
Please add yourself to the pre-merge testing project so your reviews get checked before updating the patch
clang/include/clang/Format/Format.h | ||
---|---|---|
1522 | Something is not quie right here, this text isn't ending up in the ClangFormatStyleOptions.rst |
Something is not quite right here, this text isn't ending up in the ClangFormatStyleOptions.rst
You're right, I didn't catch that before, turns out having a comment before the variable is required for dump_format_style.py to work.
I've fixed this, I'm still working on the tests, and I'll clang-format the files when it's all done.
Please clang-format the patch, I'm also getting a crash when running the tests, please make sure they pass.
I'm not sure why the tests crash, I know that when I manually test all the options for IndentExternBlock , and when testing IndentExternBlock: AfterExternBlock and setting BraceWrapping.AfterExternBlock, everything works.
i just get gibberish about loading the default LLVM style failed, and a nonsensical hex dump (0xFF 0xFE then a bunch of NULLs)
I honestly thought these crashes were unrelated.
Please add yourself to the pre-merge testing project so your reviews get checked before updating the patch
When I go there and click Watch it says:
You Shall Not Pass: #pre-merge_beta_testing
You do not have permission to edit this object.
Users with the "Can Edit" capability:
Administrators can take this action.
As for my testing, I'm doing both manual and autmoated testing, automated with ninja check all
and manual testing with main.c:
#ifdef __cplusplus
extern "C" {
#endif
void blah1(void);
#ifdef __cplusplus
}
#endif
extern "C++" {
void blah2(void) { int one = 1; }
}
and here's the command line:
~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, IndentExternBlock: true}" /Users/Marcus/Desktop/Test_Clang-Format.c
~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, IndentExternBlock: false}" /Users/Marcus/Desktop/Test_Clang-Format.c
~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, IndentExternBlock: Indent}" /Users/Marcus/Desktop/Test_Clang-Format.c
~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, IndentExternBlock: NoIndent}" /Users/Marcus/Desktop/Test_Clang-Format.c
~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, IndentExternBlock: AfterExternBlock, BraceWrapping: {AfterExternBlock: false}}" /Users/Marcus/Desktop/Test_Clang-Format.c
~/Source/External/LLVM_BUILD/bin/clang-format -i -style="{IndentWidth: 4, IndentExternBlock: AfterExternBlock, BraceWrapping: {AfterExternBlock: true}}" /Users/Marcus/Desktop/Test_Clang-Format.c
tho now that I'm manually testing it again (I really only used manual testing to make sure the options were accepted, to iterate more quickly), it looks like the AfterExternBlock: true option isn't working, but false is.
if thats true how didn't the automated tests catch it?
I've added you to https://reviews.llvm.org/project/view/78/
clang/include/clang/Format/Format.h | ||
---|---|---|
1522 | you need to add a comment before the actual variable (you can use your own choice of text) /// Option to control indenting of code inside an extern block IndentExternBlockStyle IndentExternBlock; Doing so will pull in the enum values into the ClangFormatStyleOptions.rst when running the docs/tools/dump_format_style.py and you'll get a different diff - * ``bool AfterExternBlock`` Wrap extern blocks. + * ``bool AfterExternBlock`` Wrap extern blocks; Partially superseded by IndentExternBlock .. code-block:: c++ @@ -1680,6 +1680,49 @@ the configuration (without a prefix: ``Auto``). plop(); plop(); } } +**IndentExternBlock** (``IndentExternBlockStyle``) + Option to control indenting of code inside an extern block + + Possible values: + + * ``IEBS_NoIndent`` (in configuration: ``NoIndent``) + Does not indent extern blocks. + + .. code-block:: c++ + + extern "C" { + void foo(); + } + + * ``IEBS_Indent`` (in configuration: ``Indent``) + Indents extern blocks. + + .. code-block:: c++ + + extern "C" { + void foo(); + } + + * ``IEBS_AfterExternBlock`` (in configuration: ``AfterExternBlock``) + Backwards compatible with AfterExternBlock's indenting. + AfterExternBlock: true + + .. code-block:: c++ + + extern "C" + { + void foo(); + } + AfterExternBlock: false + + .. code-block:: c++ + + extern "C" { + void foo(); + } + + + **IndentGotoLabels** (``bool``) Indent goto labels. |
if thats true how didn't the automated tests catch it?
If you are not in the premerge testing group the unit tests won't be run. (which is why I added you)
When I run the gtest it fails, I cannot see why.. but one suggestion I have is to drop the whole true/false its not like you have any backwards compatibility.
ok it crashes because you are not initializing IndentExternBlock in the getLLVMStyle() function
LLVMStyle.IndentExternBlock = FormatStyle::IEBS_NoIndent;
You cannot leave it uninitialized, now what the correct value is in my view may be an issue
I think it needs to be:
LLVMStyle.IndentExternBlock = FormatStyle::IEBS_NoIndent;
I'm wondering if the GNU style also needs
Expanded.IndentExternBlock = FormatStyle::IEBS_Indent;
I've got the indenting to work manually now as well, the issue was you need to have BreakBeforeBraces: Custom in the inline style for it to pick up BraceWrapping.AfterExternBlock's value.
Now I'm working on the automated tests, thanks for the tip about initializing, I'll look into that.
I've initialized all styles to either AfterExternBlock, if there was a BraceWrapping block, or NoIndent if there wasn't.
re-running my tests locally.
Added the style initializers, moved IEBS_AfterExternBlock to be the first enum value so that it's zero, that way the bool logic works.
Regenerated the docs as well, and also clang-formatting the files I've touched.
I reran the tests before creating this diff and everything worked.
As for crashes, none of them seem relevant; I'm on MacOS, the windows ABI crash seems especially irrelevent.
opt crashed, there were no arguments, and abort() was called.
llvm-lto2 crashed, not-prevailing.ll.tmp1-3.bc was the cause
llvm-dwarfdump crashed, arguments: -debug-line /Users/Marcus/Source/External/LLVM_BUILD/test/Object/Output/invalid.test.tmp3
llvm-as crashed, arguments: /Users/Marcus/Source/External/LLVM/llvm/test/Assembler/datalayout-invalid-stack-natural-alignment.ll
llvm-mc crashed, arguments: -triple i386-pc-win32 -filetype=obj
llvm-readobj crashed, arguments: -r /Users/Marcus/Source/External/LLVM/llvm/test/Object/Inputs/invalid-bad-section-address.coff
llc crashed, arguments: -stop-before=nonexistent -o /dev/null
This is totally fine, now I'm just concerned by the choice of defaults, I really don't know if we want to change the defaults for all the styles, I don't want to break all those people using it
One way might be if we canvas opinion from those developers who work on some of those proejcts for example @sylvestre.ledru, @Abpostelnicu what impact might this have on the Mozilla sources (if any?)
clang/lib/Format/Format.cpp | ||
---|---|---|
682 | I'm sorry but I feel these are changing the previous default which as I understood would indent ONLY if Style.BraceWrapping.AfterExternBlock == true I think in all cases other than GNU this was false, isn't that correct? | |
692 | I think you can remove this to avoid confusion that you are changing from the default LLVM style | |
697 | Isn't this changing the default? | |
711 | Isn't this changing the default? | |
725 | Ok this one feel correct. | |
893 | everyone inherits from LLVM so no need for this it only makes people think its different from the base style | |
1029 | everyone inherits from LLVM so no need for this it only makes people think its different from the base style | |
1079 | everyone inherits from LLVM so no need for this it only makes people think its different from the base style | |
1102 | everyone inherits from LLVM so no need for this it only makes people think its different from the base style | |
1121 | everyone inherits from LLVM so no need for this it only makes people think its different from the base style | |
1161 | everyone inherits from LLVM so no need for this it only makes people think its different from the base style |
clang/lib/Format/Format.cpp | ||
---|---|---|
682 | I chose IEBS_AfterExternBlock here, because it already uses the BraceWrapping.AfterExternBlock style, that way it will still use AfterExternBlock: true value a few lines lower. | |
692 | k, that makes sense; I figured nothing was specified so it should be set to no, but I can remove it also. | |
697 | Expanded.BraceWrapping.AfterExternBlock = true; is a few lines lower, so it will use that value. Maybe I should move this option to right below Expanded.BraceWrapping.AfterExternBlock = true;? | |
893 | ok, I can remove the inherited ones too. |
Ok, I've removed the inherited ones, and also removed the times I was setting a style when there wasn't one before.
also I moved the IEBS_AfterExternBlock line to right underneath the BraceWrapping.AfterExternBlock = true/false; line so it's easier to see.
and reformatted ofc.
btw, in .BraceWrapping = {true, false); blocks, AfterExternBlock is the 9th option, I checked the BraceWrapping enum.
All this confusion over the defaults is because we don't have the unit tests to check the default of each style. If we had that we could have a before/after conversation more easily
Nit: also please mark comments done once you have addressed them.
clang/lib/Format/Format.cpp | ||
---|---|---|
688 | didn't see that either yep thats correct | |
697 | totally didn't see that.. yep thats correct | |
707 | thats also correct, sorry for not seeing that | |
720 | ok thats correct too | |
776 | is this one correct? AfterExternBLock is false in this case correct? should this be NoIndent? |
btw, in .BraceWrapping = {true, false); blocks, AfterExternBlock is the 9th option, I checked the BraceWrapping enum.
This will become clearer I hope when I land D79325: [clang-format] [PR42164] Add Option to Break before While
Sorry to "go around the houses" but we'll get there in the end...I think we are close
clang/lib/Format/Format.cpp | ||
---|---|---|
776 | Yes AfterExternBlock is set to false here, but IndentExternBlock is being set to IEBS_AfterExternBlock, so the code falls back to parsing it as if IndentExternBlock wasn't set, and AfterExternBlock was set. so IEBS_AfterExternBlock works for both true and false values. Setting it to NoIndent would change the codepath to the new one and it would lose the newline between extern "C" and { in the process. |
I think we're close too.
Your other comment was interesting, about testing the styles to make sure they haven't changed with these new options, that didn't occur to me.
I think it might be worth looking into.
I'm just not really sure how we could do such a thing, or if after the option is submitted if it'd really be worth it to have run because I don't really think this option will be further modified, but the original AfterExternBlock guy didn't think anyone would want to expand that either and as a result we had to work around some of the issues with his design so who can really tell what the future holds?
This is what I was thinking about D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults
Just fixed the formatting of the ReleaseNotes.rst file, the extern blocks were slightly askew, and it might've made it a bit confusing
Made the IndentExternBlockStyle enum comments a bit clearer, and regenerated the .rst file
Format.h: indented the `AfterExternBlock: true` example code snippet with 4 spaces like the Indent option so it's more visible and matches.
I think it's perfect now.
If you want me to land this for you, I'd feel more comfortable landing it if:
a) We can land D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults first
b) The Mozilla team have tested the impact (they clang-format their entire code base I think)
I'm ok with accepting commit access, and I agree lets get D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults in, and see if Mozilla, Microsoft, Google, etc has any comments; I'm just not sure of who to ping.
Is there anything else that D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults needs? it looked pretty well fleshed out.
I need an Accept on D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults and I think @Abpostelnicu would have let us know if it failed.
clang/include/clang/Format/Format.h | ||
---|---|---|
1473 | @MarcusJohnson91 I had to make a couple of minor changes before committing
The tests ran ok especially our new one, so I don't think this has broken the default styles, we'll see if anyone else complains Thank you for the patch |
Any chance this changes could have caused this regression https://bugs.llvm.org/show_bug.cgi?id=47589 ?
I don't think so, but I can double check the style defaults for the various types, and make the code harder to mess up in a bit, currently I'm working on Clang's libSema so it might take a few days.
if it did, it's because a lot of the BraceWrapping options is just an array of like 16 true/false values so it's very unreadable, I can change that to make it more readable.
also, we can implement a second option to finish replacing the old BraceWrapping.AfterExternBlock option and completely get rid of it internally, and remap that configuration to the new split options.
After looking more closely at the issue, it seems you're having an issue with Mozilla's comment alignment option.
you want the comments to be aligned, and it appears Clang11 no longer has that option set for Mozilla's style is what you're saying?
How are you accessing Mozilla's style?
are you calling clang-format with -style=mozilla, or -style=file and you've got an implicit .clang-format somewhere?
Edit: I was able to reproduce -style=mozilla messing up comment alignment in namespaces in master, not sure where the issue could be though, we were careful to never change defaults in the original patch that introduced the change, and with the new patch I posted today.
No matter what the root cause, I still think it's a good idea to directly use the variables instead of messing around with an unwieldy bool table.
the bool table is just asking for trouble anytime BraceWrapping is expanded, or even reordered, so I'm glad that I just pushed that new patch here and hopefully it'll land soon.
as for completely cutting out the old AfterExternBlock option, I'd like to fully supersede it still, I just need a good name for wrapping the extern blocks opening curly brace, anybody got any ideas?
@MarcusJohnson91 I know it is confusing but we don't use the Mozilla coding style. We are using the Google style.
You can use the code snippet that I provided here:
https://bugs.llvm.org/show_bug.cgi?id=47589
it is easy to reproduce even without argument or configuration.
clang/lib/Format/Format.cpp | ||
---|---|---|
686 | I didn't notice this change before... I'm not a massive fan of the code on the LHS, actually it was me that added the /*XXX=*/ to make it clearer.. however If a new style is added to the BraceWrapping style then the code on the left I think will complain, but the code on the right will not and so its easy to miss a new default case. Do we need to consider that? Infact you didn't seem to change that until the very last diff! was that intentional? |
clang/lib/Format/Format.cpp | ||
---|---|---|
530–531 | I'm confused by this diff... the Review is closed, if you are proposing changes please make a new review |
clang/lib/Format/Format.cpp | ||
---|---|---|
530–531 | I went ahead and made a new revision, thanks for the tip. I've added you and Silvestre as reviewers. AS for your question about the very last diff, I originally wanted to change as little as possible, but Silvestre getting confused and thinking my patch broke something motivated me to fix the confusion with the BraceWrapping table once and for all, so here we are. |
I'm a little confused here by the use of this enum, can't it be removed now?