MarcusJohnson91 marked 2 inline comments as done.
MarcusJohnson91 added inline comments.
================
Comment at: clang/unittests/Format/FormatTest.cpp:2497
Style);
}
----------------
MyDeveloperDay wrote:
> 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
>
> https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/clang/test/SemaCXX/linkage-spec.cpp
>
> 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
>
> https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/llvm/utils/benchmark/test/donotoptimize_assembly_test.cc
>
> 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.
>
>
>
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?
================
Comment at: clang/unittests/Format/FormatTest.cpp:2505
+ verifyFormat("extern \"C\" {}", Style);
+ verifyFormat("extern \"C\" {\n int foo();\n}", Style);
+
----------------
MyDeveloperDay wrote:
> nit: these are a little easier to read when formatted as above
>
> ```
> verifyFormat("extern \"C\" {\n"
> " int foo();\n"
> "}", Style);
> ```
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75791/new/
https://reviews.llvm.org/D75791
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
