Page MenuHomePhabricator

[clang-format] Fix Microsoft style for enums
ClosedPublic

Authored by asmith on Apr 27 2020, 9:00 PM.

Details

Summary

Before this change enums were formatted incorrectly for the Microsoft style.

[C++ Example]

enum {
  one,
  two
} three, four;

[Incorrectly Formatted]

enum
{
  one,
  two
} three,
    four;

[Correct Format with Patch]

enum
{
  one,
  two
} three, four;

Diff Detail

Event Timeline

asmith created this revision.Apr 27 2020, 9:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2020, 9:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
asmith edited the summary of this revision. (Show Details)Apr 27 2020, 9:02 PM
asmith edited reviewers, added: rnk; removed: thakis.Apr 27 2020, 9:13 PM
asmith updated this revision to Diff 260542.Apr 27 2020, 10:02 PM
asmith updated this revision to Diff 260551.Apr 28 2020, 12:05 AM

Fix line breaks like this:

enum { a=1, b=2, c=3
} myEnum;

which becomes:

enum
{

a=1,
b=2,
c=3

} myEnum;

MyDeveloperDay added a project: Restricted Project.Apr 28 2020, 1:06 AM
MyDeveloperDay added a comment.EditedApr 28 2020, 1:12 AM

Before looking into the merit of the change, it needs a couple of things

  • This needs unit tests in clang/unittests/Format to demonstrate the problem and test this fixing it
  • This also needs the ClangStyleOptions.rst file generated with the clang/tools/dump_style.py
  • We should add a release note too
MyDeveloperDay requested changes to this revision.Apr 28 2020, 1:12 AM
This revision now requires changes to proceed.Apr 28 2020, 1:12 AM
MyDeveloperDay added a comment.EditedApr 28 2020, 1:45 AM

So it doesn't overcome the trailing "," issue. (which is sometimes used to put enums on multiple lines) - (NOTE: maybe it shouldn't)

---
Language: Cpp
BasedOnStyle: LLVM
AllowEnumsOnASingleLine: true
enum
{
  B,
  C,
  D,
} A, B;

vs

enum { B, C, D } A, B;

I think the name should be AllowShortEnumsOnASingleLine

As once the enum goes over a certain width it will revert to as it was before.

enum { EOF, VOID, CHAR, SHORT, DOT, NUMBER, IDENTIFIER } A;

vs

enum {
  EOF,
  VOID,
  CHAR,
  SHORT,
  INT,
  LONG,
  SIGNED,
  UNSIGNED,
  BOOL,
  FLOAT,
  DOUBLE,
  COMPLEX,
  CONST,
  RESTRICT,
  VOLATILE,
  ATOMIC,
  STRUCT,
  UNION,
  ENUM,
  LPAREN,
  RPAREN,
  LBRACKET,
  RBRACKET,
  ASTERISK,
  DOT,
  NUMBER,
  IDENTIFIER
} A;
MyDeveloperDay added inline comments.Apr 28 2020, 2:08 AM
clang/lib/Format/Format.cpp
1145

You'll need to fix up the C# tests

C:/llvm-project/clang/unittests/Format/FormatTestCSharp.cpp(48): error:       Expected: Code.str()
      Which is: "public enum var { none, @string, bool, @enum }"
To be equal to: format(Code, Style)
      Which is: "public enum var\n{\n    none,\n    @string,\n    bool,\n    @enum\n}"
With diff:
@@ -1,1 +1,7 @@
-public enum var { none, @string, bool, @enum }
+public enum var
+{
+    none,
+    @string,
+    bool,
+    @enum
+}

I think that is ok as most of the documentation seems to show them not on a single line

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/enum

clang/lib/Format/UnwrappedLineParser.cpp
1790

see below, I think you need to know this braced Initialization is part of a enum

1851

I'm wondering if this is going to impact other braced constructs?

void main()
{
    std::vector<int> list = { 1,2,3};
}

becomes

void main() {
  std::vector<int> list = {1,
  2,
  3
  };
}
2313–2320

I think you are going to have to pass down that this BracedList is for a enum only

asmith updated this revision to Diff 260732.Apr 28 2020, 12:37 PM

Rename option
Add test case
Only apply to Microsoft CPP style

asmith marked 5 inline comments as done.Apr 28 2020, 12:38 PM
asmith added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
1789

This and the line below are probably applying to more than enums. Any suggestions on a better fix?

  • This needs unit tests in clang/unittests/Format to demonstrate the problem and test this fixing it

Is there an easy way to check the test is working?

'ninja check-clang-format' passes but would like to verify.

asmith updated this revision to Diff 260812.Apr 28 2020, 6:07 PM
asmith retitled this revision from [clang-format] Fix Microsoft style for enums to [clang-format] Fix Microsoft style for C++ enums.
asmith edited the summary of this revision. (Show Details)
asmith updated this revision to Diff 260814.Apr 28 2020, 6:10 PM
MyDeveloperDay added inline comments.Apr 29 2020, 4:37 AM
clang/docs/ClangFormatStyleOptions.rst
719–720

? are you removing something?

2094–2095

unrelated change

clang/lib/Format/Format.cpp
1145

I think its ok to leave it was, just if up the unit tests

(the fact you didn't see this suggests you are not running the unit tests please ensure you are)

MyDeveloperDay requested changes to this revision.Apr 29 2020, 4:38 AM
This revision now requires changes to proceed.Apr 29 2020, 4:38 AM
asmith marked 6 inline comments as done.Apr 29 2020, 7:24 AM

The is done unless there are other comments.

clang/docs/ClangFormatStyleOptions.rst
719–720

These are changes from running the python script to regenerate the docs. Someone forgot to run the script. This info is no longer in the header.

2094–2095

Not my change. The column is too long so looks like someone manually edited the file.

clang/lib/Format/Format.cpp
1145

You need to use the latest changes. The is only applied when the style is Msft and C++. See the GetMicrosoftStyle() method.

asmith marked 3 inline comments as done.Apr 29 2020, 7:27 AM
MyDeveloperDay added inline comments.Apr 29 2020, 7:49 AM
clang/docs/ClangFormatStyleOptions.rst
719–720

If a change isn't related to your feature, simply ignore it, but don't include other changes in yours. If this has truly gone then we need to handle that separately

clang/lib/Format/Format.cpp
1145

I don't think you should change the style based on the language here, either on or off, but not changing based on language because what about "ObjectiveC"?

As I added the Microsoft style to allow C# development I think its ok to set this to what you originally had, but fix up the tests

asmith added inline comments.Apr 29 2020, 8:16 AM
clang/docs/ClangFormatStyleOptions.rst
719–720

This is generated by a script. I don't think it's a good idea to edit an automatically generated file. I can exclude the change to the RST completely and someone can regenerate the RST in a follow on patch.

clang/docs/ClangFormatStyleOptions.rst
719–720

This is from D73768: clang-format: [JS] document InsertTrailingCommas. we may need to determine why the code was not put into Format.h but that can be handled by a separate revision

asmith updated this revision to Diff 260932.Apr 29 2020, 9:03 AM
asmith retitled this revision from [clang-format] Fix Microsoft style for C++ enums to [clang-format] Fix Microsoft style for enums.
asmith edited the summary of this revision. (Show Details)

Enable for C#

asmith marked 2 inline comments as done.Apr 29 2020, 9:07 AM
asmith added inline comments.
clang/lib/Format/Format.cpp
1145

Ive enabled this for C#. I don't see the connection between ObjectiveC and Microsoft Style.

MyDeveloperDay accepted this revision.Apr 29 2020, 9:09 AM
This revision is now accepted and ready to land.Apr 29 2020, 9:09 AM
MyDeveloperDay added inline comments.Apr 29 2020, 2:10 PM
clang/lib/Format/Format.cpp
1145

The Microsoft style was a style we added to try and give us a unique style to setup for C#, but that wouldn't stop a user for using it for other languages

Saying that you want a style for C++ but not other C based languges feels a little odd

Its perfectly acceptable for someone using Objective-C to use the BasedOnStyle of Microsoft

as such I think its better for a style to have a set default, then let users modify from the default.

MyDeveloperDay requested changes to this revision.Apr 29 2020, 2:11 PM

Sorry please adjust the patch to remove the un-related changes

This revision now requires changes to proceed.Apr 29 2020, 2:11 PM
asmith updated this revision to Diff 261239.Apr 30 2020, 8:30 AM
asmith marked an inline comment as done.
asmith updated this revision to Diff 261246.Apr 30 2020, 8:40 AM
asmith updated this revision to Diff 261247.
This revision is now accepted and ready to land.Apr 30 2020, 9:10 AM

LGTM

Thanks for the help!

This revision was automatically updated to reflect the committed changes.

@asmith I'm seeing MSVC warnings from this patch:

E:\llvm\llvm-project\clang\lib\Format\UnwrappedLineParser.cpp(1475): warning C4305: 'argument': truncation from 'clang::tok::TokenKind' to 'bool'
E:\llvm\llvm-project\clang\lib\Format\UnwrappedLineParser.cpp(1828): warning C4305: 'argument': truncation from 'clang::tok::TokenKind' to 'bool'

@asmith I'm seeing MSVC warnings from this patch:

E:\llvm\llvm-project\clang\lib\Format\UnwrappedLineParser.cpp(1475): warning C4305: 'argument': truncation from 'clang::tok::TokenKind' to 'bool'
E:\llvm\llvm-project\clang\lib\Format\UnwrappedLineParser.cpp(1828): warning C4305: 'argument': truncation from 'clang::tok::TokenKind' to 'bool'

That's caused by the changed signature of parseBracedList and this change hasn't been taken into account at the above mentioned locations:

- bool parseBracedList(bool ContinueOnSemicolons = false,
+  bool parseBracedList(bool ContinueOnSemicolons = false, bool IsEnum = false, tok::TokenKind ClosingBraceKind = tok::r_brace);

BTW, it's a bit strange that a new parameter had been added in the middle of parameter list.