[PATCH] D51120: clang-format Additional Indent for class blocks

[Darby Payne via Phabricator via cfe-commits]

dpayne created this revision.
dpayne added reviewers: klimek, djasper.
Herald added subscribers: cfe-commits, mgorny.

Hi,

This is another attempt at fixing the issue describe here 
https://reviews.llvm.org/D22505. Some code bases will have an extra indent for 
member variables and functions, while the access modifiers are only indented 
once. For example,

  class MyClass
  {
  public:// 1 level of indent
  int value2;// 2 levels of indent
  private:   //1 level of indent
  int value3;// 2 levels of indent
  };

The current solution is to use a combination of increasing indent width and 
setting AccessModifierOffset to a offset. While this might work for some code 
bases, it has the downside of changing the indent level across the whole 
project. To fix this I added AdditionalIndentClassBlock, which when true will 
indent everything within a class block twice. This then could be used with 
AccessModifierOffset to achieve the desired formatting. By default it is off.

Going off the previous ticket, my goal here was to come up with a way to get 
this kind of formatting with the least amount of changes to the rest of the 
formatting rules.


Repository:
  rC Clang

https://reviews.llvm.org/D51120

Files:
  docs/ClangFormatStyleOptions.rst
  lib/Format/Format.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/CMakeLists.txt


Index: unittests/Format/CMakeLists.txt
===
--- unittests/Format/CMakeLists.txt
+++ unittests/Format/CMakeLists.txt
@@ -5,6 +5,7 @@
 add_clang_unittest(FormatTests
   CleanupTest.cpp
   FormatTest.cpp
+  FormatTestClassIndent.cpp
   FormatTestComments.cpp
   FormatTestJS.cpp
   FormatTestJava.cpp
Index: lib/Format/UnwrappedLineParser.h
===
--- lib/Format/UnwrappedLineParser.h
+++ lib/Format/UnwrappedLineParser.h
@@ -88,7 +88,7 @@
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
   void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
-  bool MunchSemi = true);
+  bool MunchSemi = true, bool ClassBlock = false);
   void parseChildBlock();
   void parsePPDirective();
   void parsePPDefine();
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -520,7 +520,7 @@
 }
 
 void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
- bool MunchSemi) {
+ bool MunchSemi, bool ClassBlock) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
  "'{' or macro block token expected");
   const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
@@ -546,6 +546,9 @@
   MustBeDeclaration);
   if (AddLevel)
 ++Line->Level;
+
+  if (Style.AdditionalIndentClassBlock && ClassBlock)
+++Line->Level;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
@@ -2126,7 +2129,7 @@
 addUnwrappedLine();
 
   parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true,
- /*MunchSemi=*/false);
+ /*MunchSemi=*/false, /*ClassBlock*/ true);
 }
   }
   // There is no addUnwrappedLine() here so that we fall through to parsing a
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -313,6 +313,7 @@
 }
 
 IO.mapOptional("AccessModifierOffset", Style.AccessModifierOffset);
+IO.mapOptional("AdditionalIndentClassBlock", 
Style.AdditionalIndentClassBlock);
 IO.mapOptional("AlignAfterOpenBracket", Style.AlignAfterOpenBracket);
 IO.mapOptional("AlignConsecutiveAssignments",
Style.AlignConsecutiveAssignments);
@@ -621,6 +622,7 @@
   FormatStyle LLVMStyle;
   LLVMStyle.Language = FormatStyle::LK_Cpp;
   LLVMStyle.AccessModifierOffset = -2;
+  LLVMStyle.AdditionalIndentClassBlock = false;
   LLVMStyle.AlignEscapedNewlines = FormatStyle::ENAS_Right;
   LLVMStyle.AlignAfterOpenBracket = FormatStyle::BAS_Align;
   LLVMStyle.AlignOperands = true;
Index: docs/ClangFormatStyleOptions.rst
===
--- docs/ClangFormatStyleOptions.rst
+++ docs/ClangFormatStyleOptions.rst
@@ -150,6 +150,9 @@
 **AccessModifierOffset** (``int``)
   The extra indent or outdent of access modifiers, e.g. ``public:``.
 
+**AdditionalIndentClassBlock** (``bool``)
+  If ``true``, adds an additional level of indention for class blocks.
+
 **AlignAfterOpenBracket** (``BracketAlignmentStyle``)
   If ``true``, horizontally aligns arguments after an open bracket.
 


Index: unittests/Format/CMakeLists.txt
===

[PATCH] D51120: clang-format Additional Indent for class blocks

[Darby Payne via Phabricator via cfe-commits]

dpayne updated this revision to Diff 162129.
dpayne added a comment.

Adding unit tests to check indents, these were mostly taken from the original 
patch https://reviews.llvm.org/D22505.


Repository:
  rC Clang

https://reviews.llvm.org/D51120

Files:
  docs/ClangFormatStyleOptions.rst
  lib/Format/Format.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/CMakeLists.txt
  unittests/Format/FormatTestClassIndent.cpp

Index: unittests/Format/FormatTestClassIndent.cpp
===
--- /dev/null
+++ unittests/Format/FormatTestClassIndent.cpp
@@ -0,0 +1,144 @@
+//===- unittest/Format/FormatTestAccess.cpp - Formatting unit tests
+//-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+
+#include "../Tooling/RewriterTestContext.h"
+#include "FormatTestUtils.h"
+
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class FormatTestClassIndent : public ::testing::Test {
+protected:
+  std::string format(llvm::StringRef Code, const FormatStyle &Style) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+std::vector Ranges(1, tooling::Range(0, Code.size()));
+bool IncompleteFormat = false;
+tooling::Replacements Replaces =
+reformat(Style, Code, Ranges, "", &IncompleteFormat);
+EXPECT_FALSE(IncompleteFormat) << Code << "\n\n";
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  FormatStyle getStyleWithAdditionalIndent() {
+FormatStyle Style = getLLVMStyle();
+Style.AdditionalIndentClassBlock = true;
+return Style;
+  }
+
+  void verifyFormat(llvm::StringRef Code, const FormatStyle & Style = getLLVMStyle()) {
+EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
+  }
+};
+
+TEST_F(FormatTestClassIndent, NoChangeWithoutAccess) {
+  verifyFormat("class A {\n"
+   "  int a1;\n"
+   "};");
+  verifyFormat("class B {\n"
+   "int b1;\n"
+   "};",
+   getStyleWithAdditionalIndent());
+}
+
+TEST_F(FormatTestClassIndent, ChangeWithAccess) {
+  verifyFormat("class C {\n"
+   "  int c1;\n"
+   "public:\n"
+   "  int c2;\n"
+   "};");
+  verifyFormat("class D {\n"
+   "int d1;\n"
+   "  public:\n"
+   "int d2;\n"
+   "};",
+   getStyleWithAdditionalIndent());
+}
+
+TEST_F(FormatTestClassIndent, MultipleAccessLevels) {
+  verifyFormat("class E {\n"
+   "  int e1;\n"
+   "public:\n"
+   "  int e2;\n"
+   "private:\n"
+   "  int e3;\n"
+   "protected:\n"
+   "  int e4;\n"
+   "};");
+  verifyFormat("class F {\n"
+   "int f1;\n"
+   "  public:\n"
+   "int f2;\n"
+   "  private:\n"
+   "int f3;\n"
+   "  protected:\n"
+   "int f4;\n"
+   "};",
+   getStyleWithAdditionalIndent());
+}
+
+TEST_F(FormatTestClassIndent, NestedClass) {
+  verifyFormat("class G {\n"
+   "  int g1;\n"
+   "  class GGA {\n"
+   "int gg1;\n"
+   "  public:\n"
+   "int gg2;\n"
+   "  };\n"
+   "public:\n"
+   "  class GGB {\n"
+   "int gg1;\n"
+   "  public:\n"
+   "int gg2;\n"
+   "  };\n"
+   "  int g2;\n"
+   "private:\n"
+   "  int g3;\n"
+   "protected:\n"
+   "  int g4;\n"
+   "};");
+  verifyFormat("class H {\n"
+   "int h1;\n"
+   "class HHA {\n"
+   "int hh1;\n"
+   "  public:\n"
+   "int hh2;\n"
+   "};\n"
+   "  public:\n"
+   "class HHB {\n"
+   "int hh1;\n"
+   "  public:\n"
+   "int hh2;\n"
+   "};\n"
+   "int h2;\n"
+   "  private:\n"
+   "int h3;\n"
+   "  protected:\n"
+   "int h4;\n"
+   "};",
+   getStyleWithAdditionalIndent());
+}
+
+} // end namespace
+} // end n

[PATCH] D43957: Fixing issue where a space was not added before a global namespace variable when SpacesInParentheses is set

[Darby Payne via Phabricator via cfe-commits]

dpayne created this revision.
dpayne added a reviewer: djasper.
Herald added subscribers: cfe-commits, klimek.

When SpacesInParentheses is set to true clang-format does not add a space 
before a global namespace variable. For example this is the output of 
clang-format for a somewhat contrived exampled.

  #include 
  
  void print_val( std::ostream &s ) { s << "Hello world" << std::endl; }
  
  int main( void ) {
print_val(::std::cout );
return 0;
  }

The .clang-format looked like

  Language:Cpp
  SpacesInParentheses: true


Repository:
  rC Clang

https://reviews.llvm.org/D43957

Files:
  lib/Format/TokenAnnotator.cpp


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2631,7 +2631,8 @@
 Style.Standard == FormatStyle::LS_Cpp03) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
   tok::kw___super, TT_TemplateCloser,
-  TT_TemplateOpener));
+  TT_TemplateOpener)) ||
+   (Left.is(tok ::l_paren) && Style.SpacesInParentheses);
   if ((Left.is(TT_TemplateOpener)) != (Right.is(TT_TemplateCloser)))
 return Style.SpacesInAngles;
   // Space before TT_StructuredBindingLSquare.


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2631,7 +2631,8 @@
 Style.Standard == FormatStyle::LS_Cpp03) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
   tok::kw___super, TT_TemplateCloser,
-  TT_TemplateOpener));
+  TT_TemplateOpener)) ||
+   (Left.is(tok ::l_paren) && Style.SpacesInParentheses);
   if ((Left.is(TT_TemplateOpener)) != (Right.is(TT_TemplateCloser)))
 return Style.SpacesInAngles;
   // Space before TT_StructuredBindingLSquare.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51120: clang-format Additional Indent for class blocks

[Darby Payne via Phabricator via cfe-commits]

dpayne updated this revision to Diff 166057.
dpayne added a comment.

Adding the missing Style member variable.


Repository:
  rC Clang

https://reviews.llvm.org/D51120

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/UnwrappedLineParser.cpp
  lib/Format/UnwrappedLineParser.h
  unittests/Format/CMakeLists.txt
  unittests/Format/FormatTestClassIndent.cpp

Index: unittests/Format/FormatTestClassIndent.cpp
===
--- /dev/null
+++ unittests/Format/FormatTestClassIndent.cpp
@@ -0,0 +1,144 @@
+//===- unittest/Format/FormatTestAccess.cpp - Formatting unit tests
+//-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+
+#include "../Tooling/RewriterTestContext.h"
+#include "FormatTestUtils.h"
+
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class FormatTestClassIndent : public ::testing::Test {
+protected:
+  std::string format(llvm::StringRef Code, const FormatStyle &Style) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+std::vector Ranges(1, tooling::Range(0, Code.size()));
+bool IncompleteFormat = false;
+tooling::Replacements Replaces =
+reformat(Style, Code, Ranges, "", &IncompleteFormat);
+EXPECT_FALSE(IncompleteFormat) << Code << "\n\n";
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  FormatStyle getStyleWithAdditionalIndent() {
+FormatStyle Style = getLLVMStyle();
+Style.AdditionalIndentClassBlock = true;
+return Style;
+  }
+
+  void verifyFormat(llvm::StringRef Code, const FormatStyle & Style = getLLVMStyle()) {
+EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
+  }
+};
+
+TEST_F(FormatTestClassIndent, NoChangeWithoutAccess) {
+  verifyFormat("class A {\n"
+   "  int a1;\n"
+   "};");
+  verifyFormat("class B {\n"
+   "int b1;\n"
+   "};",
+   getStyleWithAdditionalIndent());
+}
+
+TEST_F(FormatTestClassIndent, ChangeWithAccess) {
+  verifyFormat("class C {\n"
+   "  int c1;\n"
+   "public:\n"
+   "  int c2;\n"
+   "};");
+  verifyFormat("class D {\n"
+   "int d1;\n"
+   "  public:\n"
+   "int d2;\n"
+   "};",
+   getStyleWithAdditionalIndent());
+}
+
+TEST_F(FormatTestClassIndent, MultipleAccessLevels) {
+  verifyFormat("class E {\n"
+   "  int e1;\n"
+   "public:\n"
+   "  int e2;\n"
+   "private:\n"
+   "  int e3;\n"
+   "protected:\n"
+   "  int e4;\n"
+   "};");
+  verifyFormat("class F {\n"
+   "int f1;\n"
+   "  public:\n"
+   "int f2;\n"
+   "  private:\n"
+   "int f3;\n"
+   "  protected:\n"
+   "int f4;\n"
+   "};",
+   getStyleWithAdditionalIndent());
+}
+
+TEST_F(FormatTestClassIndent, NestedClass) {
+  verifyFormat("class G {\n"
+   "  int g1;\n"
+   "  class GGA {\n"
+   "int gg1;\n"
+   "  public:\n"
+   "int gg2;\n"
+   "  };\n"
+   "public:\n"
+   "  class GGB {\n"
+   "int gg1;\n"
+   "  public:\n"
+   "int gg2;\n"
+   "  };\n"
+   "  int g2;\n"
+   "private:\n"
+   "  int g3;\n"
+   "protected:\n"
+   "  int g4;\n"
+   "};");
+  verifyFormat("class H {\n"
+   "int h1;\n"
+   "class HHA {\n"
+   "int hh1;\n"
+   "  public:\n"
+   "int hh2;\n"
+   "};\n"
+   "  public:\n"
+   "class HHB {\n"
+   "int hh1;\n"
+   "  public:\n"
+   "int hh2;\n"
+   "};\n"
+   "int h2;\n"
+   "  private:\n"
+   "int h3;\n"
+   "  protected:\n"
+   "int h4;\n"
+   "};",
+   getStyleWithAdditionalIndent());
+}
+
+} // end namespace
+} // end namespace format
+} // end namespace clang
In

[PATCH] D51120: clang-format Additional Indent for class blocks

[Darby Payne via Phabricator via cfe-commits]

dpayne added a comment.

@djasper @klimek

Are there any comments here about this approach?


Repository:
  rC Clang

https://reviews.llvm.org/D51120



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

[PATCH] D43957: Fixing issue where a space was not added before a global namespace variable when SpacesInParentheses is set

[Darby Payne via Phabricator via cfe-commits]

dpayne updated this revision to Diff 139212.
dpayne added a comment.

Adding a unit test for formatting a global namespace var with 
SpacesInParentheses enabled.


Repository:
  rC Clang

https://reviews.llvm.org/D43957

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -8797,6 +8797,7 @@
   FormatStyle Spaces = getLLVMStyle();
 
   Spaces.SpacesInParentheses = true;
+  verifyFormat("do_something( ::globalVar );", Spaces);
   verifyFormat("call( x, y, z );", Spaces);
   verifyFormat("call();", Spaces);
   verifyFormat("std::function callback;", Spaces);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2631,7 +2631,8 @@
 Style.Standard == FormatStyle::LS_Cpp03) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
   tok::kw___super, TT_TemplateCloser,
-  TT_TemplateOpener));
+  TT_TemplateOpener)) ||
+   (Left.is(tok ::l_paren) && Style.SpacesInParentheses);
   if ((Left.is(TT_TemplateOpener)) != (Right.is(TT_TemplateCloser)))
 return Style.SpacesInAngles;
   // Space before TT_StructuredBindingLSquare.


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -8797,6 +8797,7 @@
   FormatStyle Spaces = getLLVMStyle();
 
   Spaces.SpacesInParentheses = true;
+  verifyFormat("do_something( ::globalVar );", Spaces);
   verifyFormat("call( x, y, z );", Spaces);
   verifyFormat("call();", Spaces);
   verifyFormat("std::function callback;", Spaces);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2631,7 +2631,8 @@
 Style.Standard == FormatStyle::LS_Cpp03) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
   tok::kw___super, TT_TemplateCloser,
-  TT_TemplateOpener));
+  TT_TemplateOpener)) ||
+   (Left.is(tok ::l_paren) && Style.SpacesInParentheses);
   if ((Left.is(TT_TemplateOpener)) != (Right.is(TT_TemplateCloser)))
 return Style.SpacesInAngles;
   // Space before TT_StructuredBindingLSquare.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43957: Fixing issue where a space was not added before a global namespace variable when SpacesInParentheses is set

[Darby Payne via Phabricator via cfe-commits]

dpayne added a comment.

I'll make sure to do the full diff in the future. I did that for the first 
patch but forgot to do it in the follow up patch, I won't make that mistake in 
the future.

I do not have commit access so I cannot merge myself. When you have the time 
could you merge this change?


Repository:
  rC Clang

https://reviews.llvm.org/D43957



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

[PATCH] D43957: Fixing issue where a space was not added before a global namespace variable when SpacesInParentheses is set

[Darby Payne via Phabricator via cfe-commits]

dpayne added a comment.

Hi

Bumping this commit. I do not have commit access so I cannot merge myself, if 
you have the time could you merge this commit?


Repository:
  rC Clang

https://reviews.llvm.org/D43957



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