[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Marek Kurdej via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2a42c759ae7b: [clang-format] [PR19056] Add support for 
access modifiers indentation (authored by Budovi, committed by curdeius).

Changed prior to commit:
  https://reviews.llvm.org/D94661?vs=322645=326618#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15453,6 +15453,7 @@
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
+  CHECK_PARSE_BOOL(IndentAccessModifiers);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
@@ -19155,6 +19156,57 @@
 "}",
 format(Source, Style));
 }
+
+TEST_F(FormatTest, IndentAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentAccessModifiers = true;
+  // Members are *two* levels below the record;
+  // Style.IndentWidth == 2, thus yielding a 4 spaces wide indentation.
+  verifyFormat("class C {\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("union C {\n"
+   "int i;\n"
+   "unsigned u;\n"
+   "};\n",
+   Style);
+  // Access modifiers should be indented one level below the record.
+  verifyFormat("class C {\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct S {\n"
+   "  private:\n"
+   "class C {\n"
+   "int j;\n"
+   "\n"
+   "  public:\n"
+   "C();\n"
+   "};\n"
+   "\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  // Enumerations are not records and should be unaffected.
+  Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum class E\n"
+   "{\n"
+   "  A,\n"
+   "  B\n"
+   "};\n",
+   Style);
+  // Test with a different indentation width;
+  // also proves that the result is Style.AccessModifierOffset agnostic.
+  Style.IndentWidth = 3;
+  verifyFormat("class C {\n"
+   "   public:\n"
+   "  int i;\n"
+   "};\n",
+   Style);
+}
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -85,7 +85,7 @@
   void reset();
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
-  void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
+  void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1u,
   bool MunchSemi = true);
   void parseChildBlock();
   void parsePPDirective();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -579,7 +579,7 @@
   return h;
 }
 
-void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
+void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
  bool MunchSemi) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
  "'{' or macro block token expected");
@@ -589,7 +589,7 @@
   size_t PPStartHash = computePPHash();
 
   unsigned InitialLevel = Line->Level;
-  nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
+  nextToken(/*LevelDifference=*/AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
 parseParens();
@@ -604,8 +604,7 @@
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
   MustBeDeclaration);
-  if (AddLevel)
-++Line->Level;
+  Line->Level += AddLevels;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
@@ -621,7 +620,7 @@
   size_t PPEndHash = computePPHash();
 
   // Munch the closing brace.
-  nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
+  nextToken(/*LevelDifference=*/-AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
 parseParens();

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

the LGTM, please mark your comments as done when done.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi added a comment.

In D94661#2553843 , @curdeius wrote:

> LGTM. But let other folks a few days to chime in. Especially 
> @mydeveloperday's input would be valuable here as he already attempted to 
> implement this.

Sure, no problem. Thanks for your input once again :), really appreciate it.

In D94661#2553843 , @curdeius wrote:

> Do you have have commit rights or you need somebody to land this for you? If 
> it's the latter, please provide "Firstname Name " for the commit 
> attribution or we will just use the one associated with your Phabricator user.

I don't have commit rights (this is my first contribution… well an attempt 
anyway) but the Phabricator user settings, i.e. something along `Jakub Budiský 
`, should be fine.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Marek Kurdej via Phabricator via cfe-commits]

curdeius accepted this revision.
curdeius added a comment.
This revision is now accepted and ready to land.

LGTM. But let other folks a few days to chime in. Especially @mydeveloperday's 
input would be valuable here as he already attempted to implement this.
Do you have have commit rights or you need somebody to land this for you? If 
it's the latter, please provide "Firstname Name " for the commit 
attribution or we will just use the one associated with your Phabricator user.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi updated this revision to Diff 322645.
Budovi added a comment.

Changes:

- Fixed comments

Thanks @curdeius for pointing out the issues.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15453,6 +15453,7 @@
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
+  CHECK_PARSE_BOOL(IndentAccessModifiers);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
@@ -19050,6 +19051,57 @@
 "}",
 format(Source, Style));
 }
+
+TEST_F(FormatTest, IndentAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentAccessModifiers = true;
+  // Members are *two* levels below the record;
+  // Style.IndentWidth == 2, thus yielding a 4 spaces wide indentation.
+  verifyFormat("class C {\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("union C {\n"
+   "int i;\n"
+   "unsigned u;\n"
+   "};\n",
+   Style);
+  // Access modifiers should be indented one level below the record.
+  verifyFormat("class C {\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct S {\n"
+   "  private:\n"
+   "class C {\n"
+   "int j;\n"
+   "\n"
+   "  public:\n"
+   "C();\n"
+   "};\n"
+   "\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  // Enumerations are not records and should be unaffected.
+  Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum class E\n"
+   "{\n"
+   "  A,\n"
+   "  B\n"
+   "};\n",
+   Style);
+  // Test with a different indentation width;
+  // also proves that the result is Style.AccessModifierOffset agnostic.
+  Style.IndentWidth = 3;
+  verifyFormat("class C {\n"
+   "   public:\n"
+   "  int i;\n"
+   "};\n",
+   Style);
+}
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -85,7 +85,7 @@
   void reset();
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
-  void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
+  void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1u,
   bool MunchSemi = true);
   void parseChildBlock();
   void parsePPDirective();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -579,7 +579,7 @@
   return h;
 }
 
-void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
+void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
  bool MunchSemi) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
  "'{' or macro block token expected");
@@ -589,7 +589,7 @@
   size_t PPStartHash = computePPHash();
 
   unsigned InitialLevel = Line->Level;
-  nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
+  nextToken(/*LevelDifference=*/AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
 parseParens();
@@ -604,8 +604,7 @@
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
   MustBeDeclaration);
-  if (AddLevel)
-++Line->Level;
+  Line->Level += AddLevels;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
@@ -621,7 +620,7 @@
   size_t PPEndHash = computePPHash();
 
   // Munch the closing brace.
-  nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
+  nextToken(/*LevelDifference=*/-AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
 parseParens();
@@ -1125,12 +1124,12 @@
   if (Style.BraceWrapping.AfterExternBlock) {
 addUnwrappedLine();
   }
-  parseBlock(/*MustBeDeclaration=*/true,
- 

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

It's getting into a good shape. Please fix the nits.




Comment at: clang/unittests/Format/FormatTest.cpp:19059
+  // Members are *two* levels below the record;
+  // Style.IndentWidth == 2, thus yielding a 4 spaces wide indentation
+  verifyFormat("class C {\n"

Nit: please put full stops at the end of comments. Here and elsewhere.



Comment at: clang/unittests/Format/FormatTest.cpp:19097
+  // Test with a different indentation width;
+  // also proves that the result is Style.AccessModifierOffset agonistic
+  Style.IndentWidth = 3;

Typo: agnostic.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi updated this revision to Diff 321724.
Budovi added a comment.

Changes:

- Added a unit test with a different `IndentWidth`
- Removed a unit test that contained a class nested in function; didn't add 
much IMHO
- Added brief comments about potentially non-obvious behaviour to some tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15453,6 +15453,7 @@
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
+  CHECK_PARSE_BOOL(IndentAccessModifiers);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
@@ -19050,6 +19051,57 @@
 "}",
 format(Source, Style));
 }
+
+TEST_F(FormatTest, IndentAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentAccessModifiers = true;
+  // Members are *two* levels below the record;
+  // Style.IndentWidth == 2, thus yielding a 4 spaces wide indentation
+  verifyFormat("class C {\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("union C {\n"
+   "int i;\n"
+   "unsigned u;\n"
+   "};\n",
+   Style);
+  // Access modifiers should be indented one level below the record
+  verifyFormat("class C {\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct S {\n"
+   "  private:\n"
+   "class C {\n"
+   "int j;\n"
+   "\n"
+   "  public:\n"
+   "C();\n"
+   "};\n"
+   "\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  // Enumerations are not records and should be unaffected
+  Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum class E\n"
+   "{\n"
+   "  A,\n"
+   "  B\n"
+   "};\n",
+   Style);
+  // Test with a different indentation width;
+  // also proves that the result is Style.AccessModifierOffset agonistic
+  Style.IndentWidth = 3;
+  verifyFormat("class C {\n"
+   "   public:\n"
+   "  int i;\n"
+   "};\n",
+   Style);
+}
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -85,7 +85,7 @@
   void reset();
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
-  void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
+  void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1u,
   bool MunchSemi = true);
   void parseChildBlock();
   void parsePPDirective();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -579,7 +579,7 @@
   return h;
 }
 
-void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
+void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
  bool MunchSemi) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
  "'{' or macro block token expected");
@@ -589,7 +589,7 @@
   size_t PPStartHash = computePPHash();
 
   unsigned InitialLevel = Line->Level;
-  nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
+  nextToken(/*LevelDifference=*/AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
 parseParens();
@@ -604,8 +604,7 @@
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
   MustBeDeclaration);
-  if (AddLevel)
-++Line->Level;
+  Line->Level += AddLevels;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
@@ -621,7 +620,7 @@
   size_t PPEndHash = computePPHash();
 
   // Munch the closing brace.
-  nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
+  nextToken(/*LevelDifference=*/-AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
 parseParens();
@@ -1125,12 +1124,12 @@
   if 

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi planned changes to this revision.
Budovi added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:19056
+TEST_F(FormatTest, IndentAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortEnumsOnASingleLine = false;

curdeius wrote:
> Please add an assertion that IndentWidth == 2 to make the test easier to 
> understand.
The written tests rely on several other "defaults" from the LLVM style. If 
somebody changes those defaults these unit tests (along with many others) will 
fail. This will also be true if an assertion is present to test this condition. 
It would be possible to reassign those values but this goes against how the 
rest of the tests are implemented.

But I agree that I could improve the readability by adding brief comments that 
point out important settings and the purpose of the included tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

Don't forget to mark comments as Done when... they're done :).




Comment at: clang/docs/ClangFormatStyleOptions.rst:2039
 
+**IndentAccessModifiers** (``bool``)
+  Makes an indentation level for record (``class``, ``struct``, ``union``)

Budovi wrote:
> Budovi wrote:
> > curdeius wrote:
> > > curdeius wrote:
> > > > The name `IndentAccessModifiers` is misleading for me. Access modifiers 
> > > > are public, protected, private.
> > > Hmm, actually that's the description that is misleading.
> > Hm. Will go through the bug report again to see how people describe it. I'm 
> > open to suggestions, but I'll need to fix it anyway because I see a grammar 
> > mistake.
> > 
> > The current way I tried to go about it is that, without this option, 
> > modifiers don't have their own indentation level. Without the offset given 
> > by the `AccessModifierOffset` they would just be flush with the record 
> > members.
> > 
> > The introduced option creates a level designated for the access modifiers 
> > by shifting the members further to the right.
> Hopefully the description is better this time.
Much better indeed! Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Marek Kurdej via Phabricator via cfe-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

We're getting close :).




Comment at: clang/unittests/Format/FormatTest.cpp:19056
+TEST_F(FormatTest, IndentAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortEnumsOnASingleLine = false;

Please add an assertion that IndentWidth == 2 to make the test easier to 
understand.



Comment at: clang/unittests/Format/FormatTest.cpp:19086
+   Style);
+  verifyFormat("enum class E\n"
+   "{\n"

Add a comment that `enum`s are not concerned.



Comment at: clang/unittests/Format/FormatTest.cpp:19099
+   Style);
+}
 } // namespace

Add tests with a different IndentWidth.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2039
 
+**IndentAccessModifiers** (``bool``)
+  Makes an indentation level for record (``class``, ``struct``, ``union``)

Budovi wrote:
> curdeius wrote:
> > curdeius wrote:
> > > The name `IndentAccessModifiers` is misleading for me. Access modifiers 
> > > are public, protected, private.
> > Hmm, actually that's the description that is misleading.
> Hm. Will go through the bug report again to see how people describe it. I'm 
> open to suggestions, but I'll need to fix it anyway because I see a grammar 
> mistake.
> 
> The current way I tried to go about it is that, without this option, 
> modifiers don't have their own indentation level. Without the offset given by 
> the `AccessModifierOffset` they would just be flush with the record members.
> 
> The introduced option creates a level designated for the access modifiers by 
> shifting the members further to the right.
Hopefully the description is better this time.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi updated this revision to Diff 321392.
Budovi added a comment.

Changes:

- Added release notes
- Changed the documentation (tried to make it more clear in response to the 
confusion about the added option)

Hopefully there is a full context in the patch this time.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15453,6 +15453,7 @@
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
+  CHECK_PARSE_BOOL(IndentAccessModifiers);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
@@ -19050,6 +19051,52 @@
 "}",
 format(Source, Style));
 }
+
+TEST_F(FormatTest, IndentAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortEnumsOnASingleLine = false;
+  Style.IndentAccessModifiers = true;
+  verifyFormat("class C {\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("class C {\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct S {\n"
+   "  private:\n"
+   "class C {\n"
+   "int j;\n"
+   "\n"
+   "  public:\n"
+   "C();\n"
+   "};\n"
+   "\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("union C {\n"
+   "int i;\n"
+   "unsigned u;\n"
+   "};\n",
+   Style);
+  verifyFormat("enum class E\n"
+   "{\n"
+   "  A,\n"
+   "  B\n"
+   "};\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  class C {\n"
+   "  int i;\n"
+   "  };\n"
+   "  return;\n"
+   "}\n",
+   Style);
+}
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -85,7 +85,7 @@
   void reset();
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
-  void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
+  void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1u,
   bool MunchSemi = true);
   void parseChildBlock();
   void parsePPDirective();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -579,7 +579,7 @@
   return h;
 }
 
-void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
+void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
  bool MunchSemi) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
  "'{' or macro block token expected");
@@ -589,7 +589,7 @@
   size_t PPStartHash = computePPHash();
 
   unsigned InitialLevel = Line->Level;
-  nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
+  nextToken(/*LevelDifference=*/AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
 parseParens();
@@ -604,8 +604,7 @@
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
   MustBeDeclaration);
-  if (AddLevel)
-++Line->Level;
+  Line->Level += AddLevels;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
@@ -621,7 +620,7 @@
   size_t PPEndHash = computePPHash();
 
   // Munch the closing brace.
-  nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
+  nextToken(/*LevelDifference=*/-AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
 parseParens();
@@ -1125,12 +1124,12 @@
   if (Style.BraceWrapping.AfterExternBlock) {
 addUnwrappedLine();
   }
-  parseBlock(/*MustBeDeclaration=*/true,
- /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
+  unsigned AddLevels = Style.BraceWrapping.AfterExternBlock ? 1u : 0u;
+  parseBlock(/*MustBeDeclaration=*/true, AddLevels);
 } else {
-   

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi planned changes to this revision.
Budovi added a comment.

In D94661#2537356 , 
@HazardyKnusperkeks wrote:

> You need to supply a full diff (with context).
> Please also add it to the release notes.

Oh sorry, forgot about the context lines. Will try to fix it tomorrow, along 
with the release notes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

You need to supply a full diff (with context).
Please also add it to the release notes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi updated this revision to Diff 320818.
Budovi added a comment.

Updated the tests after checking the issues were not caused by the patch and 
they are reported / fixed.

Rebased the changes to the newest version.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15453,6 +15453,7 @@
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
+  CHECK_PARSE_BOOL(IndentAccessModifiers);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
@@ -19050,6 +19051,52 @@
 "}",
 format(Source, Style));
 }
+
+TEST_F(FormatTest, IndentAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortEnumsOnASingleLine = false;
+  Style.IndentAccessModifiers = true;
+  verifyFormat("class C {\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("class C {\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("struct S {\n"
+   "  private:\n"
+   "class C {\n"
+   "int j;\n"
+   "\n"
+   "  public:\n"
+   "C();\n"
+   "};\n"
+   "\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("union C {\n"
+   "int i;\n"
+   "unsigned u;\n"
+   "};\n",
+   Style);
+  verifyFormat("enum class E\n"
+   "{\n"
+   "  A,\n"
+   "  B\n"
+   "};\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  class C {\n"
+   "  int i;\n"
+   "  };\n"
+   "  return;\n"
+   "}\n",
+   Style);
+}
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -85,7 +85,7 @@
   void reset();
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
-  void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
+  void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1u,
   bool MunchSemi = true);
   void parseChildBlock();
   void parsePPDirective();
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -579,7 +579,7 @@
   return h;
 }
 
-void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
+void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
  bool MunchSemi) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
  "'{' or macro block token expected");
@@ -589,7 +589,7 @@
   size_t PPStartHash = computePPHash();
 
   unsigned InitialLevel = Line->Level;
-  nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
+  nextToken(/*LevelDifference=*/AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
 parseParens();
@@ -604,8 +604,7 @@
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
   MustBeDeclaration);
-  if (AddLevel)
-++Line->Level;
+  Line->Level += AddLevels;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
@@ -621,7 +620,7 @@
   size_t PPEndHash = computePPHash();
 
   // Munch the closing brace.
-  nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
+  nextToken(/*LevelDifference=*/-AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
 parseParens();
@@ -1125,12 +1124,12 @@
   if (Style.BraceWrapping.AfterExternBlock) {
 addUnwrappedLine();
   }
-  parseBlock(/*MustBeDeclaration=*/true,
- /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
+  unsigned AddLevels = Style.BraceWrapping.AfterExternBlock ? 1u : 0u;
+  parseBlock(/*MustBeDeclaration=*/true, AddLevels);
 } else {
-  parseBlock(/*MustBeDeclaration=*/true,
- 

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi planned changes to this revision.
Budovi added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:17868
+  FormatStyle Style = getLLVMStyle();
+  Style.AccessModifierOffset = 4;
+  Style.AllowShortEnumsOnASingleLine = false;

Budovi wrote:
> curdeius wrote:
> > Why are those non-related style options here?
> `AccessModifierOffset` tests whether it has an influence, even thought now 
> realizing that the default value (-2) is non-zero and is probably good enough 
> as arbitrary 4. So I actually want a non-zero value for it, and this was not 
> obvious at first.
> 
> `AllowShortEnumsOnASingleLine` is needed unless I want to make the enum more 
> complex.
> 
> `BreakBeforeBraces` along with `BraceWrapping` deals with the chosen enum 
> test below, which had surprising defaults for me in the LLVMStyle. I've added 
> them as a preliminary way of "resolving" the issue with the unit test but it 
> still fails. I agree that the unit test should not try to cover the other 
> features and I will remove them and fix the test as soon as I get into it and 
> confirm that the failing test is not a bug introduced with this change and 
> open up a bug report for it.
Confirmed that the bug is present without this patch.
Related bug report: https://bugs.llvm.org/show_bug.cgi?id=46927



Comment at: clang/unittests/Format/FormatTest.cpp:17874
+  verifyFormat("class C {\n"
+   "int i;\n"
+   "};\n",

MyDeveloperDay wrote:
> So this is indented 4 because the Access modifier is 4? when the IndentWidth 
> is 2? 
> 
> but public below is indented 2.. so now I'm really confused?
The indentation is 2 levels below the class since the single level is reserved 
for access modifiers.

Access modifier does not influence the behaviour at all. The value was chosen 
arbitrarily, see my previous comment. Will reflect this in an updated patch.



Comment at: clang/unittests/Format/FormatTest.cpp:17882-17908
+  {
+const char *Expected = "struct S {\n"
+   "  private:\n"
+   "class C {\n"
+   "int j;\n"
+   "\n"
+   "  public:\n"

MyDeveloperDay wrote:
> Budovi wrote:
> > When you re-create this test using `verifyFormat`, you get a weird results, 
> > i.e. if you keep the newlines before access modifiers, the test fails 
> > because it thinks they should not be there, but if you remove them, test 
> > fails again, because the formatter includes them instead.
> > 
> > It's a good question whether it is a side-effect of `test::messUp` or a 
> > different bug, I'm not sure.
> As a drive by comment, its my experience when verifyFormat fails its often a 
> sign that there is a bug which means clang-format cannot always format from 
> arbitrary text, which really it should.
> 
> Ultimately this bug will get logged by someone who doesn't understand your 
> change as well as you do. That fix will also subtly end up creating a 
> different bug in another corner case you you haven't covered in your original 
> tests
> 
> We spiral down from there...my 2c worth.
This issue was caused by an inconsistency with access modifiers and unrelated 
to this patch. After the `test::messUp` all the newlines are stripped from the 
code, and clang-format behaved differently in a case there was no newline 
before access modifier (inserted a single one) and all the other cases (it 
inserted an additional empty line before the modifier).

The issue was fixed along with a new feature introduced 
https://reviews.llvm.org/D93846 and thus does not need reporting. The new patch 
will revert back to using `verifyFormat`.



Comment at: clang/unittests/Format/FormatTest.cpp:17914-17918
+  verifyFormat("enum class E {\n"
+   "  A,\n"
+   "  B\n"
+   "};\n",
+   Style);

Budovi wrote:
> This unit test currently fails, I believe it is a non-related bug in the 
> formatter. Nevertheless I wanted to add a check that enums (non-records) are 
> preserved.
As mentioned above, this is the bug https://bugs.llvm.org/show_bug.cgi?id=46927.
The unrelated style options will be removed in the future patch and the 
"default LLVM behaviour" tested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:17874
+  verifyFormat("class C {\n"
+   "int i;\n"
+   "};\n",

So this is indented 4 because the Access modifier is 4? when the IndentWidth is 
2? 

but public below is indented 2.. so now I'm really confused?



Comment at: clang/unittests/Format/FormatTest.cpp:17882-17908
+  {
+const char *Expected = "struct S {\n"
+   "  private:\n"
+   "class C {\n"
+   "int j;\n"
+   "\n"
+   "  public:\n"

Budovi wrote:
> When you re-create this test using `verifyFormat`, you get a weird results, 
> i.e. if you keep the newlines before access modifiers, the test fails because 
> it thinks they should not be there, but if you remove them, test fails again, 
> because the formatter includes them instead.
> 
> It's a good question whether it is a side-effect of `test::messUp` or a 
> different bug, I'm not sure.
As a drive by comment, its my experience when verifyFormat fails its often a 
sign that there is a bug which means clang-format cannot always format from 
arbitrary text, which really it should.

Ultimately this bug will get logged by someone who doesn't understand your 
change as well as you do. That fix will also subtly end up creating a different 
bug in another corner case you you haven't covered in your original tests

We spiral down from there...my 2c worth.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2039
 
+**IndentAccessModifiers** (``bool``)
+  Makes an indentation level for record (``class``, ``struct``, ``union``)

curdeius wrote:
> curdeius wrote:
> > The name `IndentAccessModifiers` is misleading for me. Access modifiers are 
> > public, protected, private.
> Hmm, actually that's the description that is misleading.
Hm. Will go through the bug report again to see how people describe it. I'm 
open to suggestions, but I'll need to fix it anyway because I see a grammar 
mistake.

The current way I tried to go about it is that, without this option, modifiers 
don't have their own indentation level. Without the offset given by the 
`AccessModifierOffset` they would just be flush with the record members.

The introduced option creates a level designated for the access modifiers by 
shifting the members further to the right.



Comment at: clang/unittests/Format/FormatTest.cpp:17868
+  FormatStyle Style = getLLVMStyle();
+  Style.AccessModifierOffset = 4;
+  Style.AllowShortEnumsOnASingleLine = false;

curdeius wrote:
> Why are those non-related style options here?
`AccessModifierOffset` tests whether it has an influence, even thought now 
realizing that the default value (-2) is non-zero and is probably good enough 
as arbitrary 4. So I actually want a non-zero value for it, and this was not 
obvious at first.

`AllowShortEnumsOnASingleLine` is needed unless I want to make the enum more 
complex.

`BreakBeforeBraces` along with `BraceWrapping` deals with the chosen enum test 
below, which had surprising defaults for me in the LLVMStyle. I've added them 
as a preliminary way of "resolving" the issue with the unit test but it still 
fails. I agree that the unit test should not try to cover the other features 
and I will remove them and fix the test as soon as I get into it and confirm 
that the failing test is not a bug introduced with this change and open up a 
bug report for it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added a comment.

That looks like a strange style option to me. Is there any bigger codebase 
formatting the code this way?




Comment at: clang/docs/ClangFormatStyleOptions.rst:2039
 
+**IndentAccessModifiers** (``bool``)
+  Makes an indentation level for record (``class``, ``struct``, ``union``)

The name `IndentAccessModifiers` is misleading for me. Access modifiers are 
public, protected, private.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2039
 
+**IndentAccessModifiers** (``bool``)
+  Makes an indentation level for record (``class``, ``struct``, ``union``)

curdeius wrote:
> The name `IndentAccessModifiers` is misleading for me. Access modifiers are 
> public, protected, private.
Hmm, actually that's the description that is misleading.



Comment at: clang/unittests/Format/FormatTest.cpp:17868
+  FormatStyle Style = getLLVMStyle();
+  Style.AccessModifierOffset = 4;
+  Style.AllowShortEnumsOnASingleLine = false;

Why are those non-related style options here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi added a comment.

In D94661#2497744 , 
@HazardyKnusperkeks wrote:

> I would add a test where you have a member before the first access modifier.

The very first unit test has no modifiers, the third one has a member before a 
modifier in the nested class C.

In D94661#2497744 , 
@HazardyKnusperkeks wrote:

> Also this is not exactly what they wanted in the bug, as far as I can see 
> members of structs or classes with no access modifier at all should only be 
> indented once.

I don't think there is a consensus about how this should actually work, see e. 
g. https://bugs.llvm.org/show_bug.cgi?id=19056#c11 .

I see qtcreator's style a bit more relevant than some astyle configuration of a 
specific project. I also find it personally ugly, can't use it myself and thus 
would have to make it configurable. In that case, this change could be viewed 
as a "possible stepping stone", even though it would probably require a major 
overhaul of the logic.

The previous patch, if I read it correctly, implements this detail in the same 
way as I did.




Comment at: clang/unittests/Format/FormatTest.cpp:17882-17908
+  {
+const char *Expected = "struct S {\n"
+   "  private:\n"
+   "class C {\n"
+   "int j;\n"
+   "\n"
+   "  public:\n"

When you re-create this test using `verifyFormat`, you get a weird results, 
i.e. if you keep the newlines before access modifiers, the test fails because 
it thinks they should not be there, but if you remove them, test fails again, 
because the formatter includes them instead.

It's a good question whether it is a side-effect of `test::messUp` or a 
different bug, I'm not sure.



Comment at: clang/unittests/Format/FormatTest.cpp:17914-17918
+  verifyFormat("enum class E {\n"
+   "  A,\n"
+   "  B\n"
+   "};\n",
+   Style);

This unit test currently fails, I believe it is a non-related bug in the 
formatter. Nevertheless I wanted to add a check that enums (non-records) are 
preserved.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

I would add a test where you have a member before the first access modifier.
Also this is not exactly what they wanted in the bug, as far as I can see 
members of structs or classes with no access modifier at all should only be 
indented once.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94661/new/

https://reviews.llvm.org/D94661

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94661: [clang-format] [PR19056] Add support for access modifiers indentation

[Jakub Budiský via Phabricator via cfe-commits]

Budovi created this revision.
Budovi added reviewers: MyDeveloperDay, rsmith.
Budovi added a project: clang-format.
Budovi requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Adds support for coding styles that make a separate indentation level for 
access modifiers, such as Code::Blocks or QtCreator.

The new option, `IndentAccessModifiers`, if enabled, forces the content inside 
classes, structs and unions (“records”) to be indented twice while removing a 
level for access modifiers. The value of `AccessModifierOffset` is disregarded 
in this case, aiming towards an ease of use.




The PR (https://bugs.llvm.org/show_bug.cgi?id=19056) had an implementation 
attempt by @MyDeveloperDay already (https://reviews.llvm.org/D60225) but I've 
decided to start from scratch. They differ in functionality, chosen approaches, 
and even the option name. The code tries to re-use the existing functionality 
to achieve this behavior, limiting possibility of breaking something else.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94661

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -14279,6 +14279,7 @@
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
+  CHECK_PARSE_BOOL(IndentAccessModifiers);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
@@ -17862,6 +17863,68 @@
Style));
 }
 
+TEST_F(FormatTest, IndentAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AccessModifierOffset = 4;
+  Style.AllowShortEnumsOnASingleLine = false;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = false;
+  Style.IndentAccessModifiers = true;
+  verifyFormat("class C {\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  verifyFormat("class C {\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n",
+   Style);
+  {
+const char *Expected = "struct S {\n"
+   "  private:\n"
+   "class C {\n"
+   "int j;\n"
+   "\n"
+   "  public:\n"
+   "C();\n"
+   "};\n"
+   "\n"
+   "  public:\n"
+   "int i;\n"
+   "};\n";
+const char *ToFormat = "struct S {\n"
+   "private:\n"
+   "class C {\n"
+   "int j;\n"
+   "public:\n"
+   "C();\n"
+   "};\n"
+   "public:\n"
+   "int i;\n"
+   "};\n";
+
+EXPECT_EQ(Expected, format(ToFormat, Style));
+EXPECT_EQ(Expected, format(Expected, Style));
+  }
+  verifyFormat("union C {\n"
+   "int i;\n"
+   "unsigned u;\n"
+   "};\n",
+   Style);
+  verifyFormat("enum class E {\n"
+   "  A,\n"
+   "  B\n"
+   "};\n",
+   Style);
+  verifyFormat("void foo() {\n"
+   "  class C {\n"
+   "  int i;\n"
+   "  };\n"
+   "  return;\n"
+   "}\n",
+   Style);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -85,7 +85,7 @@
   void reset();
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
-  void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
+  void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1u,
   bool MunchSemi = true);
   void parseChildBlock();
   void parsePPDirective(unsigned Level);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -579,7 +579,7 @@
   return h;
 }
 
-void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
+void UnwrappedLineParser::parseBlock(bool