Page MenuHomePhabricator

[clang-format] Added new option IndentExternBlock
ClosedPublic

Authored by MarcusJohnson91 on Mar 6 2020, 6:05 PM.

Details

Summary

Replaced the table form with dubious comments with the actual variables

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I've fixed all of your comments as well as fixed the tests.

MarcusJohnson91 edited the summary of this revision. (Show Details)

Did everything you asked and did a littl bit of my own cleanup as well.

Removed forgotten comment from control logic of UnwrappedLineParser

MyDeveloperDay accepted this revision.May 16 2020, 10:08 AM

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
216

I don't think that's necessary

This revision is now accepted and ready to land.May 16 2020, 10:08 AM
MarcusJohnson91 marked an inline comment as done.May 16 2020, 5:58 PM

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.

LGTM

So what's the next step? I've never committed to LLVM before.

LGTM

So what's the next step? I've never committed to LLVM before.

In my mind you have two choices

  1. require commit access
  2. 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 ;-) )

MyDeveloperDay added a comment.EditedMay 17 2020, 8:50 AM

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

https://reviews.llvm.org/project/view/78/

clang/include/clang/Format/Format.h
1555 ↗(On Diff #264462)

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

https://reviews.llvm.org/project/view/78/

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?

Fixed the generation of ReleaseNotes.rst

clang/include/clang/Format/Format.h
1555 ↗(On Diff #264462)

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.

Sorry forgot to submit the comment issue, seems like you worked that bit out already

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.

MarcusJohnson91 added a comment.EditedMay 18 2020, 6:49 PM

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
755

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?

766

I think you can remove this to avoid confusion that you are changing from the default LLVM style

771

Isn't this changing the default?

786

Isn't this changing the default?

801

Ok this one feel correct.

1002

everyone inherits from LLVM so no need for this it only makes people think its different from the base style

1145

everyone inherits from LLVM so no need for this it only makes people think its different from the base style

1194

everyone inherits from LLVM so no need for this it only makes people think its different from the base style

1216

everyone inherits from LLVM so no need for this it only makes people think its different from the base style

1234

everyone inherits from LLVM so no need for this it only makes people think its different from the base style

1275

everyone inherits from LLVM so no need for this it only makes people think its different from the base style

MarcusJohnson91 marked 4 inline comments as done.May 19 2020, 3:24 AM
MarcusJohnson91 added inline comments.
clang/lib/Format/Format.cpp
755

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.

766

k, that makes sense; I figured nothing was specified so it should be set to no, but I can remove it also.

771

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;?

1002

ok, I can remove the inherited ones too.

MarcusJohnson91 updated this revision to Diff 264839.EditedMay 19 2020, 3:37 AM

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
761

didn't see that either yep thats correct

771

totally didn't see that.. yep thats correct

781

thats also correct, sorry for not seeing that

795

ok thats correct too

862

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

MarcusJohnson91 marked 10 inline comments as done.May 19 2020, 6:10 AM
MarcusJohnson91 marked 5 inline comments as done.May 19 2020, 6:16 AM
MarcusJohnson91 added inline comments.
clang/lib/Format/Format.cpp
862

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.

MarcusJohnson91 marked an inline comment as done.May 19 2020, 6:17 AM

Sorry to "go around the houses" but we'll get there in the end...I think we are close

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?

MyDeveloperDay accepted this revision.May 19 2020, 7:23 AM

I think this LGTM now...

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.

Let's first see we don't break anything on mozilla.

Abpostelnicu accepted this revision.May 19 2020, 8:12 AM

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)

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.

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.

This revision was automatically updated to reflect the committed changes.
MyDeveloperDay added inline comments.May 20 2020, 1:49 PM
clang/include/clang/Format/Format.h
1531 ↗(On Diff #265337)

@MarcusJohnson91 I had to make a couple of minor changes before committing

  1. it needed rebasing (because of changes from this morning)
  2. these \code statements need to be indented by 3 spaces otherwise it can't see the beginning and end of the code segment and would have failed the doc build
  3. There were a couple of extra blank lines in the release notes (I actually think that was my mistake this morning) but it failed the rest lint check that I use

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 ?

MarcusJohnson91 added a comment.EditedSep 20 2020, 1:55 PM

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.

MarcusJohnson91 edited the summary of this revision. (Show Details)
MarcusJohnson91 added a comment.EditedSep 21 2020, 1:43 PM

@sylvestre.ledru

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
746

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
586

I'm confused by this diff... the Review is closed, if you are proposing changes please make a new review

MarcusJohnson91 marked an inline comment as done.Sep 22 2020, 3:28 AM
MarcusJohnson91 marked an inline comment as not done.Sep 22 2020, 3:38 AM
MarcusJohnson91 added inline comments.
clang/lib/Format/Format.cpp
586

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.