[PATCH] D151047: [clang-format] Fix indent for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added inline comments.



Comment at: clang/unittests/Format/FormatTestSelective.cpp:643-646
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+ "#define REF(alias) alias alias_var;\n"
+ "}"; // Format this line only

owenpan wrote:
> Then I'd remove `Style.FixNamespaceComments` as a factor by formatting a 
> non-brace line.
The problem I observed did not trigger by formatting some arbitrary line. It 
triggered when formatting specifically the line with the closing `}`. 
Formatting any other line makes therefore no sense. I just tried it again.
`FixNamespaceComments = false` means that the expected result is that the code 
fragment remains unchanged. With `FixNamespaceComments = true`, the formatter 
would change it (by adding the `// namespace ns` comment after the brace).

If you are still offended by the test, I will give up and will remove it. There 
are still the clang rename tests after all that execute the particular code 
path and uncovered the problem originally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added a comment.

If everyone is ok with the changes, could someone commit it for me since I do 
not have commit rights? Thanks!
`Sedenion <39583823+sedeni...@users.noreply.github.com>`

Note: I cannot really make sense of the error reported by the pre merge checks 
. I suspect a problem in 
the build system. I can apply the patch locally on the latest main branch 
without any conflicts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D155094: Refactoring and asserts in LevelIndentTracker. (NFC)

[Sedenion via Phabricator via cfe-commits]

Sedeniono created this revision.
Sedeniono added reviewers: owenpan, MyDeveloperDay, HazardyKnusperkeks.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added a reviewer: rymiel.
Sedeniono requested review of this revision.

- adjustToUnmodifiedLine: The code does something only for non-PP-directives. 
This is now reflected by putting the if-check to the top. This also ensures 
that the assert() there is executed only if IndentForLevel is actually accessed.

- getIndent(): assert valud index into IndentForLevel.

- Added explanation regarding the intention of IndentForLevel.

This refactoring was split from https://reviews.llvm.org/D151047.
But both patches are independent of each other.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155094

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -88,13 +88,13 @@
   /// level to the same indent.
   /// Note that \c nextLine must have been called before this method.
   void adjustToUnmodifiedLine(const AnnotatedLine &Line) {
-unsigned LevelIndent = Line.First->OriginalColumn;
-if (static_cast(LevelIndent) - Offset >= 0)
-  LevelIndent -= Offset;
-assert(Line.Level < IndentForLevel.size());
-if ((!Line.First->is(tok::comment) || IndentForLevel[Line.Level] == -1) &&
-!Line.InPPDirective) {
-  IndentForLevel[Line.Level] = LevelIndent;
+if (!Line.InPPDirective) {
+  unsigned LevelIndent = Line.First->OriginalColumn;
+  if (static_cast(LevelIndent) - Offset >= 0)
+LevelIndent -= Offset;
+  assert(Line.Level < IndentForLevel.size());
+  if (Line.First->isNot(tok::comment) || IndentForLevel[Line.Level] == -1)
+IndentForLevel[Line.Level] = LevelIndent;
 }
   }
 
@@ -148,6 +148,7 @@
   /// at \p IndentForLevel[l], or a value < 0 if the indent for
   /// that level is unknown.
   unsigned getIndent(unsigned Level) const {
+assert(Level < IndentForLevel.size());
 if (IndentForLevel[Level] != -1)
   return IndentForLevel[Level];
 if (Level == 0)
@@ -159,7 +160,10 @@
   const AdditionalKeywords &Keywords;
   const unsigned AdditionalIndent;
 
-  /// The indent in characters for each level.
+  /// The indent in characters for each level. It remembers the indent of
+  /// previous lines (that are not PP directives) of equal or lower levels. 
This
+  /// is used to align formatted lines to the indent of previous non-formatted
+  /// lines. Think about the --lines parameter of clang-format.
   SmallVector IndentForLevel;
 
   /// Offset of the current line relative to the indent level.


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -88,13 +88,13 @@
   /// level to the same indent.
   /// Note that \c nextLine must have been called before this method.
   void adjustToUnmodifiedLine(const AnnotatedLine &Line) {
-unsigned LevelIndent = Line.First->OriginalColumn;
-if (static_cast(LevelIndent) - Offset >= 0)
-  LevelIndent -= Offset;
-assert(Line.Level < IndentForLevel.size());
-if ((!Line.First->is(tok::comment) || IndentForLevel[Line.Level] == -1) &&
-!Line.InPPDirective) {
-  IndentForLevel[Line.Level] = LevelIndent;
+if (!Line.InPPDirective) {
+  unsigned LevelIndent = Line.First->OriginalColumn;
+  if (static_cast(LevelIndent) - Offset >= 0)
+LevelIndent -= Offset;
+  assert(Line.Level < IndentForLevel.size());
+  if (Line.First->isNot(tok::comment) || IndentForLevel[Line.Level] == -1)
+IndentForLevel[Line.Level] = LevelIndent;
 }
   }
 
@@ -148,6 +148,7 @@
   /// at \p IndentForLevel[l], or a value < 0 if the indent for
   /// that level is unknown.
   unsigned getIndent(unsigned Level) const {
+assert(Level < IndentForLevel.size());
 if (IndentForLevel[Level] != -1)
   return IndentForLevel[Level];
 if (Level == 0)
@@ -159,7 +160,10 @@
   const AdditionalKeywords &Keywords;
   const unsigned AdditionalIndent;
 
-  /// The indent in characters for each level.
+  /// The indent in characters for each level. It remembers the indent of
+  /// previous lines (that are not PP directives) of equal or lower levels. This
+  /// is used to align formatted lines to the indent of previous non-formatted
+  /// lines. Think about the --lines parameter of clang-format.
   SmallVector IndentForLevel;
 
   /// Offset of the current line relative to the indent level.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 539583.
Sedeniono added a comment.

@owenpan Oh wow, you are right. Phabricator shows that there are two separate 
commits, under "Revision Contents" > "Commits", and their commit messages. But 
apparently there is no way to download some proper *.patch file that contains 
the meta data, only the total diff.

So, I uploaded a new diff here (D151047 ): It 
contains the bugfix. I will also edit the summary at the top with the 
corresponding commit message.

I have submitted the refactoring commit separately here: 
https://reviews.llvm.org/D155094

Note that both patches (D151047  and D155094 
) are fortunately independent of each other. 
I checked that both can be applied to the latest main branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -528,6 +528,26 @@
 format("  int a;\n"
"void ff() {}",
11, 0));
+
+  // https://github.com/llvm/llvm-project/issues/59178
+  Style = getMozillaStyle();
+  EXPECT_EQ("int a()\n"
+"{\n"
+"return 0;\n"
+"}\n"
+"int b()\n"
+"{\n"
+"  return 42;\n"
+"}",
+format("int a()\n"
+   "{\n"
+   "return 0;\n"
+   "}\n"
+   "int b()\n"
+   "{\n"
+   "return 42;\n" // Format this line only
+   "}",
+   32, 0));
 }
 
 TEST_F(FormatTestSelective, UnderstandsTabs) {
@@ -617,6 +637,70 @@
  "namespace ns1 { namespace ns2 {\n"
  "}}";
   EXPECT_EQ(Code, format(Code, 0, 0));
+
+  Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+ "#define REF(alias) alias alias_var;\n"
+ "}";
+  EXPECT_EQ(Code, format(Code, 51, 1));
+}
+
+TEST_F(FormatTestSelective, FormatMacroRegardlessOfPreviousIndent) {
+  // clang-format currently does not (or should not) take into account the
+  // indent of previous unformatted lines when formatting a PP directive.
+  // Technically speaking, LevelIndentTracker::IndentForLevel is only for non-PP
+  // lines. So these tests here check that the indent of previous non-PP lines
+  // do not affect the formatting. If this requirement changes, the tests here
+  // need to be adapted.
+  Style = getLLVMStyle();
+
+  Style.IndentPPDirectives = FormatStyle::PPDirectiveIndentStyle::PPDIS_None;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#define some\n" // Formatted line
+"#endif\n"   // That this line is also formatted might be a bug.
+"}};", // Dito: Bug?
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_BeforeHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"  #define some\n" // Formatted line
+" #endif\n"
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_AfterHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#  define some\n" // Formatted line
+"#endif\n" // That this line is also formatted might be a bug.
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/Unw

[PATCH] D151047: [clang-format] Fix indent for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked 4 inline comments as done.
Sedeniono added inline comments.



Comment at: clang/unittests/Format/FormatTestSelective.cpp:641-646
+  Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+ "#define REF(alias) alias alias_var;\n"
+ "}";
+  EXPECT_EQ(Code, format(Code, 51, 1));

owenpan wrote:
> I suppose this would make the purpose of the test more clear. However, this 
> new test (in either version) would pass without the patch, so it doesn't 
> capture the bug mentioned in D151047#4369742?
When I originally submitted the fix for the incorrect selective formatting and 
you committed it to the main branch, some clang rename unit tests failed 
because they ran into some `assert()` in `LevelIndentTracker` (see [this 
post](https://reviews.llvm.org/D151047#4366917)), making a revert necessary. 
There was no test in the format Google Tests themselves that caught that 
mistake. Hence I added this test. But this also means that this test passes 
with and without the current patch. See an [earlier 
comment](https://reviews.llvm.org/D151047#4369742) where I describe the problem 
in more details.

Note that the change in `LineJoiner::join()` (which triggered the problem) is 
no longer in the current version of the patch because of some other unrelated 
changes that happened in the main branch since then. Again, please compare 
[another earlier comment](https://reviews.llvm.org/D151047#4396727).



Comment at: clang/unittests/Format/FormatTestSelective.cpp:649
+
+TEST_F(FormatTestSelective, FormatMacroRegardlessOfPreviousIndent) {
+  // clang-format currently does not (or should not) take into account the

owenpan wrote:
> Again, these tests would pass without the patch.
As above, these tests fail with the original first submitted and incomplete 
fix. As far as I can remember, it happens if you forget the `if 
(!Line.InPPDirective)` condition in the patch. The result is that the PP 
directives end up being formatted "randomly". No other tests in the formatting 
test suite so far checked the selective PP directive formatting (at least I got 
no test failures with the incomplete patch). So yes, the new tests pass with 
and without the patch, but they are still worthwhile.
Also compare [this earlier comment](https://reviews.llvm.org/D151047#4449996).



Comment at: clang/unittests/Format/FormatTestSelective.cpp:658
+
+  Style.IndentPPDirectives = FormatStyle::PPDirectiveIndentStyle::PPDIS_None;
+  EXPECT_EQ("  class Foo {\n"

owenpan wrote:
> It's the default and can be deleted.
I think setting it explicitly makes it clearer which setting is the important 
one that is under test here.



Comment at: clang/unittests/Format/FormatTestSelective.cpp:664
+"#endif\n"   // That this line is also formatted might be a 
bug.
+"}};", // Dito: Bug?
+format("  class Foo {\n"

owenpan wrote:
> Typo.
I cannot see any typo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D155094: Refactoring and asserts in LevelIndentTracker. (NFC)

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 540682.
Sedeniono added a comment.

Implemented review changes suggested by @owenpan.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155094

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -88,14 +88,15 @@
   /// level to the same indent.
   /// Note that \c nextLine must have been called before this method.
   void adjustToUnmodifiedLine(const AnnotatedLine &Line) {
+if (Line.InPPDirective)
+  return;
+assert(Line.Level < IndentForLevel.size());
+if (Line.First->is(tok::comment) && IndentForLevel[Line.Level] != -1)
+  return;
 unsigned LevelIndent = Line.First->OriginalColumn;
 if (static_cast(LevelIndent) - Offset >= 0)
   LevelIndent -= Offset;
-assert(Line.Level < IndentForLevel.size());
-if ((!Line.First->is(tok::comment) || IndentForLevel[Line.Level] == -1) &&
-!Line.InPPDirective) {
-  IndentForLevel[Line.Level] = LevelIndent;
-}
+IndentForLevel[Line.Level] = LevelIndent;
   }
 
 private:
@@ -148,6 +149,7 @@
   /// at \p IndentForLevel[l], or a value < 0 if the indent for
   /// that level is unknown.
   unsigned getIndent(unsigned Level) const {
+assert(Level < IndentForLevel.size());
 if (IndentForLevel[Level] != -1)
   return IndentForLevel[Level];
 if (Level == 0)
@@ -159,7 +161,10 @@
   const AdditionalKeywords &Keywords;
   const unsigned AdditionalIndent;
 
-  /// The indent in characters for each level.
+  /// The indent in characters for each level. It remembers the indent of
+  /// previous lines (that are not PP directives) of equal or lower levels. 
This
+  /// is used to align formatted lines to the indent of previous non-formatted
+  /// lines. Think about the --lines parameter of clang-format.
   SmallVector IndentForLevel;
 
   /// Offset of the current line relative to the indent level.


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -88,14 +88,15 @@
   /// level to the same indent.
   /// Note that \c nextLine must have been called before this method.
   void adjustToUnmodifiedLine(const AnnotatedLine &Line) {
+if (Line.InPPDirective)
+  return;
+assert(Line.Level < IndentForLevel.size());
+if (Line.First->is(tok::comment) && IndentForLevel[Line.Level] != -1)
+  return;
 unsigned LevelIndent = Line.First->OriginalColumn;
 if (static_cast(LevelIndent) - Offset >= 0)
   LevelIndent -= Offset;
-assert(Line.Level < IndentForLevel.size());
-if ((!Line.First->is(tok::comment) || IndentForLevel[Line.Level] == -1) &&
-!Line.InPPDirective) {
-  IndentForLevel[Line.Level] = LevelIndent;
-}
+IndentForLevel[Line.Level] = LevelIndent;
   }
 
 private:
@@ -148,6 +149,7 @@
   /// at \p IndentForLevel[l], or a value < 0 if the indent for
   /// that level is unknown.
   unsigned getIndent(unsigned Level) const {
+assert(Level < IndentForLevel.size());
 if (IndentForLevel[Level] != -1)
   return IndentForLevel[Level];
 if (Level == 0)
@@ -159,7 +161,10 @@
   const AdditionalKeywords &Keywords;
   const unsigned AdditionalIndent;
 
-  /// The indent in characters for each level.
+  /// The indent in characters for each level. It remembers the indent of
+  /// previous lines (that are not PP directives) of equal or lower levels. This
+  /// is used to align formatted lines to the indent of previous non-formatted
+  /// lines. Think about the --lines parameter of clang-format.
   SmallVector IndentForLevel;
 
   /// Offset of the current line relative to the indent level.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151047: [clang-format] Fix indent for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 540683.
Sedeniono marked 4 inline comments as done.
Sedeniono added a comment.

Fixed typo in comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -528,6 +528,26 @@
 format("  int a;\n"
"void ff() {}",
11, 0));
+
+  // https://github.com/llvm/llvm-project/issues/59178
+  Style = getMozillaStyle();
+  EXPECT_EQ("int a()\n"
+"{\n"
+"return 0;\n"
+"}\n"
+"int b()\n"
+"{\n"
+"  return 42;\n"
+"}",
+format("int a()\n"
+   "{\n"
+   "return 0;\n"
+   "}\n"
+   "int b()\n"
+   "{\n"
+   "return 42;\n" // Format this line only
+   "}",
+   32, 0));
 }
 
 TEST_F(FormatTestSelective, UnderstandsTabs) {
@@ -617,6 +637,70 @@
  "namespace ns1 { namespace ns2 {\n"
  "}}";
   EXPECT_EQ(Code, format(Code, 0, 0));
+
+  Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+ "#define REF(alias) alias alias_var;\n"
+ "}";
+  EXPECT_EQ(Code, format(Code, 51, 1));
+}
+
+TEST_F(FormatTestSelective, FormatMacroRegardlessOfPreviousIndent) {
+  // clang-format currently does not (or should not) take into account the
+  // indent of previous unformatted lines when formatting a PP directive.
+  // Technically speaking, LevelIndentTracker::IndentForLevel is only for non-PP
+  // lines. So these tests here check that the indent of previous non-PP lines
+  // do not affect the formatting. If this requirement changes, the tests here
+  // need to be adapted.
+  Style = getLLVMStyle();
+
+  Style.IndentPPDirectives = FormatStyle::PPDirectiveIndentStyle::PPDIS_None;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#define some\n" // Formatted line
+"#endif\n"   // That this line is also formatted might be a bug.
+"}};", // Ditto: Bug?
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_BeforeHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"  #define some\n" // Formatted line
+" #endif\n"
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_AfterHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#  define some\n" // Formatted line
+"#endif\n" // That this line is also formatted might be a bug.
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -74,6 +74,13 @@
: Line.Level * PPIndentWidth;
   Indent += AdditionalIndent;
 } else {
+  // When going to lower levels, forget previous higher levels so that we
+  // recompute future higher levels. But don't forget them if we enter a PP
+  // directive, since these do not terminate a C++ code block.
+  if (!Line.InPPDirective) {
+assert(Line.Level <= IndentForLevel.size());
+IndentForLevel.resize(Line.Level + 1);
+  }
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
___
cfe

[PATCH] D151047: [clang-format] Fix indent for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added inline comments.



Comment at: clang/unittests/Format/FormatTestSelective.cpp:664
+"#endif\n"   // That this line is also formatted might be a 
bug.
+"}};", // Dito: Bug?
+format("  class Foo {\n"

Sedeniono wrote:
> owenpan wrote:
> > Typo.
> I cannot see any typo.
Oh, ok, ditto, not dito. Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indent for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked 6 inline comments as done.
Sedeniono added inline comments.



Comment at: clang/unittests/Format/FormatTestSelective.cpp:641-646
+  Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+ "#define REF(alias) alias alias_var;\n"
+ "}";
+  EXPECT_EQ(Code, format(Code, 51, 1));

owenpan wrote:
> Sedeniono wrote:
> > owenpan wrote:
> > > I suppose this would make the purpose of the test more clear. However, 
> > > this new test (in either version) would pass without the patch, so it 
> > > doesn't capture the bug mentioned in D151047#4369742?
> > When I originally submitted the fix for the incorrect selective formatting 
> > and you committed it to the main branch, some clang rename unit tests 
> > failed because they ran into some `assert()` in `LevelIndentTracker` (see 
> > [this post](https://reviews.llvm.org/D151047#4366917)), making a revert 
> > necessary. There was no test in the format Google Tests themselves that 
> > caught that mistake. Hence I added this test. But this also means that this 
> > test passes with and without the current patch. See an [earlier 
> > comment](https://reviews.llvm.org/D151047#4369742) where I describe the 
> > problem in more details.
> > 
> > Note that the change in `LineJoiner::join()` (which triggered the problem) 
> > is no longer in the current version of the patch because of some other 
> > unrelated changes that happened in the main branch since then. Again, 
> > please compare [another earlier 
> > comment](https://reviews.llvm.org/D151047#4396727).
> Then what do you think about changing the test case as suggested?
I now added the hyperlink. Also, added the "format this line only" comment, but 
on the `"}"` line, since that is the line that gets formatted. Also changed the 
length parameter to `format()` from 1 to 0; this never made any sense to me (a 
range of 0 length?), but it is done everywhere else, so whatever. Removing the 
`Style.FixNamespaceComments = false;` line would be wrong because the value is 
`true` for the LLVM style, so I kept it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indent for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 541146.
Sedeniono marked an inline comment as done.
Sedeniono added a comment.

Implemented review changes as suggested by @owenpan.

If this is finally ok, someone please commit it to main using the following:
`Sedenion <39583823+sedeni...@users.noreply.github.com>`
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -528,6 +528,26 @@
 format("  int a;\n"
"void ff() {}",
11, 0));
+
+  // https://github.com/llvm/llvm-project/issues/59178
+  Style = getMozillaStyle();
+  EXPECT_EQ("int a()\n"
+"{\n"
+"return 0;\n"
+"}\n"
+"int b()\n"
+"{\n"
+"  return 42;\n"
+"}",
+format("int a()\n"
+   "{\n"
+   "return 0;\n"
+   "}\n"
+   "int b()\n"
+   "{\n"
+   "return 42;\n" // Format this line only
+   "}",
+   32, 0));
 }
 
 TEST_F(FormatTestSelective, UnderstandsTabs) {
@@ -617,6 +637,61 @@
  "namespace ns1 { namespace ns2 {\n"
  "}}";
   EXPECT_EQ(Code, format(Code, 0, 0));
+
+  // https://reviews.llvm.org/D151047#4369742
+  Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+ "#define REF(alias) alias alias_var;\n"
+ "}"; // Format this line only
+  EXPECT_EQ(Code, format(Code, 51, 0));
+}
+
+TEST_F(FormatTestSelective, FormatMacroRegardlessOfPreviousIndent) {
+  // clang-format currently does not (or should not) take into account the
+  // indent of previous unformatted lines when formatting a PP directive.
+  // Technically speaking, LevelIndentTracker::IndentForLevel is only for non-PP
+  // lines. So these tests here check that the indent of previous non-PP lines
+  // do not affect the formatting. If this requirement changes, the tests here
+  // need to be adapted.
+  Style = getLLVMStyle();
+
+  const StringRef Code{"  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};"};
+
+  EXPECT_EQ(Style.IndentPPDirectives,
+FormatStyle::PPDirectiveIndentStyle::PPDIS_None);
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#define some\n" // Formatted line
+"#endif\n"   // That this line is also formatted might be a bug.
+"}};", // Ditto: Bug?
+format(Code, 57, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_BeforeHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"  #define some\n" // Formatted line
+" #endif\n"
+"}};",
+format(Code, 57, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_AfterHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#  define some\n" // Formatted line
+"#endif\n" // That this line is also formatted might be a bug.
+"}};",
+format(Code, 57, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -74,6 +74,13 @@
: Line.Level * PPIndentWidth;
   Indent += AdditionalIndent;
 } else {
+  // When going to lower levels, forget previous higher levels so that we
+  // recompute future higher levels. But don't forget them if we enter a PP
+  // directive, since these do not terminate a C++ code block.
+  if (!Line.InPPDirective) {
+assert(Line.Level <= IndentForLevel.size());
+IndentForLevel.resize(Line.Level + 1);
+  }
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155094: Refactoring and asserts in LevelIndentTracker. (NFC)

[Sedenion via Phabricator via cfe-commits]

Sedeniono added a comment.

I don't see how the failed test cases  could 
be my fault; they deal with the address sanitizer.
So maybe some one could please commit this to main using `Sedenion 
<39583823+sedeni...@users.noreply.github.com>`? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155094

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

[PATCH] D153228: [clang-format] Fixed bad performance with enabled qualifier fixer.

[Sedenion via Phabricator via cfe-commits]

Sedeniono created this revision.
Sedeniono added a reviewer: MyDeveloperDay.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan.
Sedeniono requested review of this revision.

This fixes github issue #57117: If the "QualifierAlignment"
option of clang-format is set to anything else but "Leave", the
"QualifierAlignmentFixer" pass gets enabled. This pass scales
quadratically with the number of preprocessor branches, i.e.
with the number of elements in TokenAnalyzer::UnwrappedLines.
The reason is that QualifierAlignmentFixer::process() generates
the UnwrappedLines, but then QualifierAlignmentFixer::analyze()
calls LeftRightQualifierAlignmentFixer::process() several times
(once for each qualifier) which again each time generates the
UnwrappedLines.

This commit gets rid of this double loop by registering the
individual LeftRightQualifierAlignmentFixer passes directly in
the top most container of passes (local variable "Passes" in
reformat()).
With this change, the original example in the github issue #57117
now takes only around 3s instead of >300s to format.

Since QualifierAlignmentFixer::analyze() got deleted, we also
no longer have the code with the NonNoOpFixes. This causes
replacements that end up not changing anything to appear in the
list of final replacements. There is a unit test to check that
this does not happen: QualifierFixerTest.NoOpQualifierReplacements.
However, it got broken at some point in time. So this commit
fixes the test. To keep the behavior that no no-op replacements
should appear from the qualifier fixer, the corresponding code
from QualifierAlignmentFixer::analyze() was moved to the top
reformat() function. Thus, is now done for **every** replacement
of every formatting pass. If no-op replacements are a problem
for the qualifier fixer, then it seems to be a good idea to
filter them out always.

See
https://github.com/llvm/llvm-project/issues/57117#issuecomment-1546716934
for some more details.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153228

Files:
  clang/lib/Format/Format.cpp
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/lib/Format/QualifierAlignmentFixer.h
  clang/unittests/Format/QualifierFixerTest.cpp

Index: clang/unittests/Format/QualifierFixerTest.cpp
===
--- clang/unittests/Format/QualifierFixerTest.cpp
+++ clang/unittests/Format/QualifierFixerTest.cpp
@@ -1016,8 +1016,8 @@
   std::vector Left;
   std::vector Right;
   std::vector ConfiguredTokens;
-  QualifierAlignmentFixer::PrepareLeftRightOrdering(Style.QualifierOrder, Left,
-Right, ConfiguredTokens);
+  prepareLeftRightOrderingForQualifierAlignmentFixer(Style.QualifierOrder, Left,
+ Right, ConfiguredTokens);
 
   EXPECT_EQ(Left.size(), (size_t)2);
   EXPECT_EQ(Right.size(), (size_t)2);
@@ -1181,10 +1181,12 @@
   Style.QualifierAlignment = FormatStyle::QAS_Custom;
   Style.QualifierOrder = {"static", "const", "type"};
 
-  ReplacementCount = 0;
-  EXPECT_EQ(ReplacementCount, 0);
   verifyFormat("static const uint32 foo[] = {0, 31};", Style);
+  EXPECT_EQ(ReplacementCount, 0);
+
   verifyFormat("#define MACRO static const", Style);
+  EXPECT_EQ(ReplacementCount, 0);
+
   verifyFormat("using sc = static const", Style);
   EXPECT_EQ(ReplacementCount, 0);
 }
Index: clang/lib/Format/QualifierAlignmentFixer.h
===
--- clang/lib/Format/QualifierAlignmentFixer.h
+++ clang/lib/Format/QualifierAlignmentFixer.h
@@ -25,32 +25,13 @@
 const Environment &)>
 AnalyzerPass;
 
-class QualifierAlignmentFixer : public TokenAnalyzer {
-  // Left to Right ordering requires multiple passes
-  SmallVector Passes;
-  StringRef &Code;
-  ArrayRef Ranges;
-  unsigned FirstStartColumn;
-  unsigned NextStartColumn;
-  unsigned LastStartColumn;
-  StringRef FileName;
+void addQualifierAlignmentFixerPasses(const FormatStyle &Style,
+  SmallVector &Passes);
 
-public:
-  QualifierAlignmentFixer(const Environment &Env, const FormatStyle &Style,
-  StringRef &Code, ArrayRef Ranges,
-  unsigned FirstStartColumn, unsigned NextStartColumn,
-  unsigned LastStartColumn, StringRef FileName);
-
-  std::pair
-  analyze(TokenAnnotator &Annotator,
-  SmallVectorImpl &AnnotatedLines,
-  FormatTokenLexer &Tokens) override;
-
-  static void PrepareLeftRightOrdering(const std::vector &Order,
-   std::vector &LeftOrder,
-   std::vector &RightOrder,
-   std::vector &Qualifiers);
-};
+void prepareLeftRightOrderingForQualifierAlignmentFixer(
+const std::vector &Order, std::vec

[PATCH] D153228: [clang-format] Fixed bad performance with enabled qualifier fixer.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 532721.
Sedeniono added a comment.

As suggested by HazardyKnusperkeks, increased the number of stack elements and 
removed the reserve(). That causes all Passes to be stored on the stack. (I 
counted 17 distinct possible passes, but not all of them can be active at the 
same time: Some are javascript and some a C++ exclusive.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153228

Files:
  clang/lib/Format/Format.cpp
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/lib/Format/QualifierAlignmentFixer.h
  clang/unittests/Format/QualifierFixerTest.cpp

Index: clang/unittests/Format/QualifierFixerTest.cpp
===
--- clang/unittests/Format/QualifierFixerTest.cpp
+++ clang/unittests/Format/QualifierFixerTest.cpp
@@ -1016,8 +1016,8 @@
   std::vector Left;
   std::vector Right;
   std::vector ConfiguredTokens;
-  QualifierAlignmentFixer::PrepareLeftRightOrdering(Style.QualifierOrder, Left,
-Right, ConfiguredTokens);
+  prepareLeftRightOrderingForQualifierAlignmentFixer(Style.QualifierOrder, Left,
+ Right, ConfiguredTokens);
 
   EXPECT_EQ(Left.size(), (size_t)2);
   EXPECT_EQ(Right.size(), (size_t)2);
@@ -1181,10 +1181,12 @@
   Style.QualifierAlignment = FormatStyle::QAS_Custom;
   Style.QualifierOrder = {"static", "const", "type"};
 
-  ReplacementCount = 0;
-  EXPECT_EQ(ReplacementCount, 0);
   verifyFormat("static const uint32 foo[] = {0, 31};", Style);
+  EXPECT_EQ(ReplacementCount, 0);
+
   verifyFormat("#define MACRO static const", Style);
+  EXPECT_EQ(ReplacementCount, 0);
+
   verifyFormat("using sc = static const", Style);
   EXPECT_EQ(ReplacementCount, 0);
 }
Index: clang/lib/Format/QualifierAlignmentFixer.h
===
--- clang/lib/Format/QualifierAlignmentFixer.h
+++ clang/lib/Format/QualifierAlignmentFixer.h
@@ -25,32 +25,13 @@
 const Environment &)>
 AnalyzerPass;
 
-class QualifierAlignmentFixer : public TokenAnalyzer {
-  // Left to Right ordering requires multiple passes
-  SmallVector Passes;
-  StringRef &Code;
-  ArrayRef Ranges;
-  unsigned FirstStartColumn;
-  unsigned NextStartColumn;
-  unsigned LastStartColumn;
-  StringRef FileName;
+void addQualifierAlignmentFixerPasses(const FormatStyle &Style,
+  SmallVector &Passes);
 
-public:
-  QualifierAlignmentFixer(const Environment &Env, const FormatStyle &Style,
-  StringRef &Code, ArrayRef Ranges,
-  unsigned FirstStartColumn, unsigned NextStartColumn,
-  unsigned LastStartColumn, StringRef FileName);
-
-  std::pair
-  analyze(TokenAnnotator &Annotator,
-  SmallVectorImpl &AnnotatedLines,
-  FormatTokenLexer &Tokens) override;
-
-  static void PrepareLeftRightOrdering(const std::vector &Order,
-   std::vector &LeftOrder,
-   std::vector &RightOrder,
-   std::vector &Qualifiers);
-};
+void prepareLeftRightOrderingForQualifierAlignmentFixer(
+const std::vector &Order, std::vector &LeftOrder,
+std::vector &RightOrder,
+std::vector &Qualifiers);
 
 class LeftRightQualifierAlignmentFixer : public TokenAnalyzer {
   std::string Qualifier;
Index: clang/lib/Format/QualifierAlignmentFixer.cpp
===
--- clang/lib/Format/QualifierAlignmentFixer.cpp
+++ clang/lib/Format/QualifierAlignmentFixer.cpp
@@ -25,18 +25,13 @@
 namespace clang {
 namespace format {
 
-QualifierAlignmentFixer::QualifierAlignmentFixer(
-const Environment &Env, const FormatStyle &Style, StringRef &Code,
-ArrayRef Ranges, unsigned FirstStartColumn,
-unsigned NextStartColumn, unsigned LastStartColumn, StringRef FileName)
-: TokenAnalyzer(Env, Style), Code(Code), Ranges(Ranges),
-  FirstStartColumn(FirstStartColumn), NextStartColumn(NextStartColumn),
-  LastStartColumn(LastStartColumn), FileName(FileName) {
+void addQualifierAlignmentFixerPasses(const FormatStyle &Style,
+  SmallVector &Passes) {
   std::vector LeftOrder;
   std::vector RightOrder;
   std::vector ConfiguredQualifierTokens;
-  PrepareLeftRightOrdering(Style.QualifierOrder, LeftOrder, RightOrder,
-   ConfiguredQualifierTokens);
+  prepareLeftRightOrderingForQualifierAlignmentFixer(
+  Style.QualifierOrder, LeftOrder, RightOrder, ConfiguredQualifierTokens);
 
   // Handle the left and right alignment separately.
   for (const auto &Qualifier : LeftOrder) {
@@ -59,51 +54,6 @@
   }
 }
 
-std::pair QualifierAlignmentFixer::analyze(
-TokenAnnotator & /*Annotator*/,
-SmallVect

[PATCH] D153228: [clang-format] Fixed bad performance with enabled qualifier fixer.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked 2 inline comments as done.
Sedeniono added inline comments.



Comment at: clang/lib/Format/Format.cpp:3475
   AnalyzerPass;
   SmallVector Passes;
 

HazardyKnusperkeks wrote:
> Just increase here, and add a comment that there are multiple passes added in 
> `addQualifierAlignmentFixerPasses`.
> 
> I think the idea was and should be that we always store the pointers on the 
> stack.
Ok, done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153228

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

[PATCH] D153243: [clang-format] Don't finalize #if, #else, #endif, etc.

[Sedenion via Phabricator via cfe-commits]

Sedeniono requested changes to this revision.
Sedeniono added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:1416-1421
+  if (Tok->is(tok::hash) && !Tok->Previous && Tok->Next &&
+  Tok->Next->isOneOf(tok::pp_if, tok::pp_ifdef, tok::pp_ifndef,
+ tok::pp_elif, tok::pp_elifdef, tok::pp_elifndef,
+ tok::pp_else, tok::pp_endif)) {
+Tok = Tok->Next;
+  }

After some time, I found your comment 
[here](https://reviews.llvm.org/D150057#inline-1449546) which explains the 
intention of the "do not finalize '#'" thing. Would it be possible to provide 
the reasoning here as some comment, or at least some hint that it has to do 
with multiple passes because of PP branches? To give future readers some chance 
to understand what is going on. Maybe something along the lines of
```
// Never mark the '#' of a PP branch as finalized, since clang-format performs 
// multiple passes, where each pass represents a version where the code of 
// one of the PP branches is filled in. Later passes must also  be allowed to 
// format the PP tokens.
```
Alternatively or additionally, I would be happy with a link to [your 
explanation](https://reviews.llvm.org/D150057#inline-1449546) in the code or 
the commit message, if the llvm rules/conventions allow it.

Apart from this, I have dug way too little into how clang-format works to 
reason about the change. I tested it with the full hpp file from 
https://github.com/llvm/llvm-project/issues/57117, and can confirm that the 
issue is fixed. So I believe you. ;-)
But: Considering your comment 
[here](https://reviews.llvm.org/D150057#inline-1449546), I thought that the 
whole "do not finalize '#'" thing is relevant only if there are some 
"else"-PP-branches? But in the minimal example/the unit test, there is no 
"else" branch? How do the multiple passes because of PP branches relate to the 
`clang-format off` comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153243

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

[PATCH] D153228: [clang-format] Fixed bad performance with enabled qualifier fixer.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked an inline comment as done.
Sedeniono added inline comments.



Comment at: clang/lib/Format/Format.cpp:3571-3585
+  // Don't make replacements that replace nothing. This can affect e.g. the
+  // output of clang-format with the --output-replacements-xml option.
+  tooling::Replacements NonNoOpFixes;
+  for (const tooling::Replacement &Fix : Fixes) {
+StringRef OriginalCode = Code.substr(Fix.getOffset(), Fix.getLength());
+if (!OriginalCode.equals(Fix.getReplacementText())) {
+  auto Err = NonNoOpFixes.add(Fix);

owenpan wrote:
> You only need this for the `QualifierAlignment` passes as others (e.g. 
> `IntegerLiteralSeparatorFixer`) already skip no-op replacements.
Yes, in principle I need to do this only after all QualifierAlignment passes 
ran. The key point is that it needs to be done only after **all** 
QualifierAlignment passes ran. In other words, I cannot move this to 
`LeftRightQualifierAlignmentFixer::analyze()`; it would do nothing there. The 
non-no-op code exists so that if e.g. `const volatile` becomes `volatile const` 
in one pass and then again `const volatile` in another pass, we end up with no 
replacements.

I also cannot simply put all the QualifierAlignment passes into a simple 
dedicated loop that directly runs them after one another; the part with 
`applyAllReplacements()` etc (line 3554 till 3566 above) would be missing 
between the passes. I could, of course, extract lines 3554 till 3566 into a new 
function and call it in both places. Should I do that?

Or do you mean that I should wrap the NonNoOpFixes code in an `if 
(Style.QualifierAlignment != FormatStyle::QAS_Leave)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153228

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

[PATCH] D153228: [clang-format] Fixed bad performance with enabled qualifier fixer.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 534024.
Sedeniono added a comment.

As suggested by the reviewers, the noop fixes are now filtered only if the 
qualifier alignment passes are active. Also implemented the other minor 
refactorings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153228

Files:
  clang/lib/Format/Format.cpp
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/lib/Format/QualifierAlignmentFixer.h
  clang/unittests/Format/QualifierFixerTest.cpp

Index: clang/unittests/Format/QualifierFixerTest.cpp
===
--- clang/unittests/Format/QualifierFixerTest.cpp
+++ clang/unittests/Format/QualifierFixerTest.cpp
@@ -1016,8 +1016,8 @@
   std::vector Left;
   std::vector Right;
   std::vector ConfiguredTokens;
-  QualifierAlignmentFixer::PrepareLeftRightOrdering(Style.QualifierOrder, Left,
-Right, ConfiguredTokens);
+  prepareLeftRightOrderingForQualifierAlignmentFixer(Style.QualifierOrder, Left,
+ Right, ConfiguredTokens);
 
   EXPECT_EQ(Left.size(), (size_t)2);
   EXPECT_EQ(Right.size(), (size_t)2);
@@ -1181,10 +1181,12 @@
   Style.QualifierAlignment = FormatStyle::QAS_Custom;
   Style.QualifierOrder = {"static", "const", "type"};
 
-  ReplacementCount = 0;
-  EXPECT_EQ(ReplacementCount, 0);
   verifyFormat("static const uint32 foo[] = {0, 31};", Style);
+  EXPECT_EQ(ReplacementCount, 0);
+
   verifyFormat("#define MACRO static const", Style);
+  EXPECT_EQ(ReplacementCount, 0);
+
   verifyFormat("using sc = static const", Style);
   EXPECT_EQ(ReplacementCount, 0);
 }
Index: clang/lib/Format/QualifierAlignmentFixer.h
===
--- clang/lib/Format/QualifierAlignmentFixer.h
+++ clang/lib/Format/QualifierAlignmentFixer.h
@@ -25,32 +25,13 @@
 const Environment &)>
 AnalyzerPass;
 
-class QualifierAlignmentFixer : public TokenAnalyzer {
-  // Left to Right ordering requires multiple passes
-  SmallVector Passes;
-  StringRef &Code;
-  ArrayRef Ranges;
-  unsigned FirstStartColumn;
-  unsigned NextStartColumn;
-  unsigned LastStartColumn;
-  StringRef FileName;
+void addQualifierAlignmentFixerPasses(const FormatStyle &Style,
+  SmallVectorImpl &Passes);
 
-public:
-  QualifierAlignmentFixer(const Environment &Env, const FormatStyle &Style,
-  StringRef &Code, ArrayRef Ranges,
-  unsigned FirstStartColumn, unsigned NextStartColumn,
-  unsigned LastStartColumn, StringRef FileName);
-
-  std::pair
-  analyze(TokenAnnotator &Annotator,
-  SmallVectorImpl &AnnotatedLines,
-  FormatTokenLexer &Tokens) override;
-
-  static void PrepareLeftRightOrdering(const std::vector &Order,
-   std::vector &LeftOrder,
-   std::vector &RightOrder,
-   std::vector &Qualifiers);
-};
+void prepareLeftRightOrderingForQualifierAlignmentFixer(
+const std::vector &Order, std::vector &LeftOrder,
+std::vector &RightOrder,
+std::vector &Qualifiers);
 
 class LeftRightQualifierAlignmentFixer : public TokenAnalyzer {
   std::string Qualifier;
Index: clang/lib/Format/QualifierAlignmentFixer.cpp
===
--- clang/lib/Format/QualifierAlignmentFixer.cpp
+++ clang/lib/Format/QualifierAlignmentFixer.cpp
@@ -25,18 +25,13 @@
 namespace clang {
 namespace format {
 
-QualifierAlignmentFixer::QualifierAlignmentFixer(
-const Environment &Env, const FormatStyle &Style, StringRef &Code,
-ArrayRef Ranges, unsigned FirstStartColumn,
-unsigned NextStartColumn, unsigned LastStartColumn, StringRef FileName)
-: TokenAnalyzer(Env, Style), Code(Code), Ranges(Ranges),
-  FirstStartColumn(FirstStartColumn), NextStartColumn(NextStartColumn),
-  LastStartColumn(LastStartColumn), FileName(FileName) {
+void addQualifierAlignmentFixerPasses(const FormatStyle &Style,
+  SmallVectorImpl &Passes) {
   std::vector LeftOrder;
   std::vector RightOrder;
   std::vector ConfiguredQualifierTokens;
-  PrepareLeftRightOrdering(Style.QualifierOrder, LeftOrder, RightOrder,
-   ConfiguredQualifierTokens);
+  prepareLeftRightOrderingForQualifierAlignmentFixer(
+  Style.QualifierOrder, LeftOrder, RightOrder, ConfiguredQualifierTokens);
 
   // Handle the left and right alignment separately.
   for (const auto &Qualifier : LeftOrder) {
@@ -59,51 +54,6 @@
   }
 }
 
-std::pair QualifierAlignmentFixer::analyze(
-TokenAnnotator & /*Annotator*/,
-SmallVectorImpl & /*AnnotatedLines*/,
-FormatTokenLexer & /*Tokens*/) {
-  auto Env = Environment::make(Code, FileName, Ranges, 

[PATCH] D153228: [clang-format] Fixed bad performance with enabled qualifier fixer.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked 6 inline comments as done.
Sedeniono added inline comments.



Comment at: clang/lib/Format/Format.cpp:3476-3477
+
+  // Regarding the 16: Note that multiple passes are added in
+  // addQualifierAlignmentFixerPasses().
+  SmallVector Passes;

HazardyKnusperkeks wrote:
> owenpan wrote:
> > This comment is unnecessary and too verbose IMO. I suggest that it be 
> > deleted.
> > This comment is unnecessary and too verbose IMO. I suggest that it be 
> > deleted.
> 
> I think one should note that there are more passes added than directly meets 
> the eye here.
After looking at the code again, I decided to remove the comment. I think the 
"structure" of the code below that adds the passes already highlights that 
there is something "special" going on with the qualifier alignment passes, 
since they aren't added via a lambda.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153228

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked an inline comment as done.
Sedeniono added a comment.

@owenpan , @MyDeveloperDay Any opinion on the latest changes? 
Otherwise, since I do not have commit rights, someone needs to commit the 
changes to main (my name and mail: `Sedenion 
<39583823+sedeni...@users.noreply.github.com>`). Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D153228: [clang-format] Fixed bad performance with enabled qualifier fixer.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked an inline comment as done.
Sedeniono added a comment.

In D153228#4446523 , @MyDeveloperDay 
wrote:

> For us to land this for you we'll need your name and email

Please use: `Sedenion <39583823+sedeni...@users.noreply.github.com>`
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153228

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 534688.
Sedeniono added a comment.

As suggested by @owenpan, split the commits differently. My previous statement, 
that the whole fix consists of a revert of D129064 
 was not entirely correct: The fix 
reintroduces the `resize()` that got removed in D129064 
, but it additionally surrounds it by the `if 
(!Line.InPPDirective)`. Otherwise, the formatting is wrong if some PP 
directives are encountered. (The FormatMacroRegardlessOfPreviousIndent tests 
check this.)
Also, as @owenpan suggested, I replaced the `!Line.First->is(tok::comment)` 
with `Line.First->isNot(tok::comment)` in `adjustToUnmodifiedLine()`.
I also rebased the patch to the current main branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -528,6 +528,26 @@
 format("  int a;\n"
"void ff() {}",
11, 0));
+
+  // https://github.com/llvm/llvm-project/issues/59178
+  Style = getMozillaStyle();
+  EXPECT_EQ("int a()\n"
+"{\n"
+"return 0;\n"
+"}\n"
+"int b()\n"
+"{\n"
+"  return 42;\n"
+"}",
+format("int a()\n"
+   "{\n"
+   "return 0;\n"
+   "}\n"
+   "int b()\n"
+   "{\n"
+   "return 42;\n" // Format this line only
+   "}",
+   32, 0));
 }
 
 TEST_F(FormatTestSelective, UnderstandsTabs) {
@@ -617,6 +637,70 @@
  "namespace ns1 { namespace ns2 {\n"
  "}}";
   EXPECT_EQ(Code, format(Code, 0, 0));
+
+  Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+ "#define REF(alias) alias alias_var;\n"
+ "}";
+  EXPECT_EQ(Code, format(Code, 51, 1));
+}
+
+TEST_F(FormatTestSelective, FormatMacroRegardlessOfPreviousIndent) {
+  // clang-format currently does not (or should not) take into account the
+  // indent of previous unformatted lines when formatting a PP directive.
+  // Technically speaking, LevelIndentTracker::IndentForLevel is only for non-PP
+  // lines. So these tests here check that the indent of previous non-PP lines
+  // do not affect the formatting. If this requirement changes, the tests here
+  // need to be adapted.
+  Style = getLLVMStyle();
+
+  Style.IndentPPDirectives = FormatStyle::PPDirectiveIndentStyle::PPDIS_None;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#define some\n" // Formatted line
+"#endif\n"   // That this line is also formatted might be a bug.
+"}};", // Dito: Bug?
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_BeforeHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"  #define some\n" // Formatted line
+" #endif\n"
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_AfterHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#  define some\n" // Formatted line
+"#endif\n" // That this line is also formatted might be a bug.
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.c

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono created this revision.
Sedeniono added a reviewer: curdeius.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.
Sedeniono requested review of this revision.

Fixes github issues #59178 and #58464.

The problem was that the LevelIndentTracker remembered
the indentation level of previous deeper levels when
leaving a scope. Afterwards, when it entered again a
deeper level, it blindly reused the the previous
indentation level. In case of the --lines option
configured such that the previous deeper level was not
formatted, that previous level was whatever happened
to be there in the source code. The formatter simply
believed it.

This is fixed by letting the LevelIndentTracker forget
the previous deeper levels when stepping out of them
(=> change in LevelIndentTracker::nextLine()).
Note that this used to be the case until LLVM 14.0.6,
but was changed in
https://github.com/llvm/llvm-project/issues/56352 to
fix a crash. Our commit here essentially reverts that
crash fix. It seemed to have been incorrect. The proper
fix is to set the AnnotedLine::Level of joined lines
correctly (=> change in LineJoiner::join()).

See
https://github.com/llvm/llvm-project/issues/59178#issuecomment-1542637781
for some more details.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151047

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp


Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -528,6 +528,26 @@
 format("  int a;\n"
"void ff() {}",
11, 0));
+
+  // https://github.com/llvm/llvm-project/issues/59178
+  Style = getMozillaStyle();
+  EXPECT_EQ("int a()\n"
+"{\n"
+"return 0;\n"
+"}\n"
+"int b()\n"
+"{\n"
+"  return 42;\n"
+"}",
+format("int a()\n"
+   "{\n"
+   "return 0;\n"
+   "}\n"
+   "int b()\n"
+   "{\n"
+   "return 42;\n" // Format this line only
+   "}",
+   32, 0));
 }
 
 TEST_F(FormatTestSelective, UnderstandsTabs) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -74,6 +74,12 @@
: Line.Level * PPIndentWidth;
   Indent += AdditionalIndent;
 } else {
+  // When going to lower levels, forget previous higher levels so that we
+  // recompute future higher levels. But don't forget them if we enter a PP
+  // directive, since these do not terminate a code block.
+  if (!Line.InPPDirective)
+IndentForLevel.resize(Line.Level + 1);
+
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
@@ -910,6 +916,7 @@
   Tok->TotalLength += LengthA;
   A.Last = Tok;
 }
+A.Level = B.Level;
   }
 
   const FormatStyle &Style;


Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -528,6 +528,26 @@
 format("  int a;\n"
"void ff() {}",
11, 0));
+
+  // https://github.com/llvm/llvm-project/issues/59178
+  Style = getMozillaStyle();
+  EXPECT_EQ("int a()\n"
+"{\n"
+"return 0;\n"
+"}\n"
+"int b()\n"
+"{\n"
+"  return 42;\n"
+"}",
+format("int a()\n"
+   "{\n"
+   "return 0;\n"
+   "}\n"
+   "int b()\n"
+   "{\n"
+   "return 42;\n" // Format this line only
+   "}",
+   32, 0));
 }
 
 TEST_F(FormatTestSelective, UnderstandsTabs) {
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -74,6 +74,12 @@
: Line.Level * PPIndentWidth;
   Indent += AdditionalIndent;
 } else {
+  // When going to lower levels, forget previous higher levels so that we
+  // recompute future higher levels. But don't forget them if we enter a PP
+  // directive, since these do not terminate a code block.
+  if (!Line.InPPDirective)
+IndentForLevel.resize(Line.Level + 1);
+
   Indent = getIndent(Line.Level);

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added a comment.

The build failures seem to be unrelated to my changes. The builds of other 
reviews also show them, and the error messages point to things I haven't 
changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added a comment.

@MyDeveloperDay Thanks for the positive review. 
Regarding https://github.com/llvm/llvm-project/issues/56352 and the Beyoncé 
Rule, the original fix for the crash actually added a test (see the changes 
made at that time 
).
 The test also still works, since it crashes/debug-asserts with just the 
reintroduction of the `resize()`. That caused me to dig some more and I came up 
with the `A.Level = B.Level;` part of the fix to fix the crash. The `if 
(!Line.InPPDirective)` part was because of another failing test.

Since this is my first submission to llvm, I do not have commit access. So 
someone else has to commit and push this into main.
Regarding name and mail: If you use "Sedenion" as name and 
"39583823+sedeni...@users.noreply.github.com" as mail, I guess github will 
attribute the commit to my github account ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added a comment.

@owenpan Here is the name and mail in the <> format:
`Sedenion <39583823+sedeni...@users.noreply.github.com>`

@HazardyKnusperkeks Ok, I will remember it for the next one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added a comment.

Ok, sorry, the test failures are on me. There is a lesson learned for me: I 
naively ran only the formatter unit tests, not the whole test suite of llvm. I 
somehow assumed that my code changes only affected clang-format, since the 
UnwrappedLineFormatter is in the "Format" folder. I will need some time to 
investigate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added a comment.

Heh, ok, so I wasn't that naive then to not run the tests of everything :-)

I had a look at the issue. The ClangRenameTests first do some replacements, and 
then call formatAndApplyReplacements() 

 with them, which also formats the lines containing the replacements. With my 
patch, calling `clang-format.exe -style=llvm --lines=13:13` on the following 
file (line 13 is the `OldAlias old_alias;`)

  #include "header.h"
  
  namespace x { class Old {}; }
  namespace ns {
  #define REF(alias) alias alias_var;
  
  #define ALIAS(old) \
using old##Alias = x::old; \
REF(old##Alias);
  
  ALIAS(Old);
  
  OldAlias old_alias;
  }

triggers the assert in adjustToUnmodifiedLine() 
.
 This is what happens in the `RenameAliasTest.AliasesInMacros` test.

The issue originates already from the line `#define REF(alias) alias 
alias_var;`: These are two `AnnotatedLine` instances at first, namely `#define 
REF(alias)` with `AnnotatedLine::Level` set to 0 and `alias alias_var;` with 
`AnnotatedLine::Level` equals to 1. They get joined eventually. Since my patch 
sets `A.Level = B.Level;` in `join()`, the joined line gets level 1. But the 
`LevelIndentTracker` is still in level 0 (i.e. `IndentForLevel` contains only 1 
element).

It is kind of the same problem why I added the `if (!Line.InPPDirective)` check 
in `nextLine()` in the patch: I think, if both AnnotatedLines are PP 
directives, then `join()` should not step "into" or "out of" levels. 
PP-directives are kind-of "temporary insertions". So, replacing in `join()` the 
`A.Level = B.Level;` from my patch with

  assert(A.InPPDirective == B.InPPDirective);
  if (!A.InPPDirective && !B.InPPDirective)
A.Level = B.Level;

seems to fix the problem. At least all Google tests (including the 
ClangRenameTests) pass that didn't fail without the patch. Any opinions on that 
idea?

I have to think some more about the levels and what it means regarding 
`join()`. I also need to get the clang-tidy test to run locally and also add 
the above example as a formatter test (if possible in a reduced form). Most 
likely I won't be able to do this until next week, sorry. :-/

To create a new fix, do I understand the guide 
 correctly that I 
basically execute `arc diff --verbatim` and mention `Depends on D151047` 
somewhere in the message? Will it then appear here automatically?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

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

Ok, here is the 2nd attempt at fixing the issue. The patch is based on the 
current main branch.

Regarding `LineJoiner::join()`: Turns out, not setting `A.Level` but reverting 
the original crash fix of https://reviews.llvm.org/D129064 no longer produces 
the crash. From debugging it I guess this is because of 
https://reviews.llvm.org/D144296 (although I haven't bisected it to confirm 
this). I am not really sure whether setting `A.Level` should or should not be 
done. So I am keeping it as it is.

But therefore note, since https://reviews.llvm.org/D144296 seems to not have 
been merged into the LLVM 16, the present fix can probably not be merged 
without further changes.

So, the new patch basically just reverts the original fix of 
https://reviews.llvm.org/D129064 (i.e. it re-introduces the `resize()` in 
`LevelIndentTracker::nextLine()`). The other changes are just cosmetics and 
additional tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 528531.
Sedeniono added a comment.
Herald added subscribers: llvm-commits, pcwang-thead.
Herald added a project: LLVM.

Fixes github issues #59178, #58464 and #62799.

  

The problem was that the LevelIndentTracker remembered
the indentation level of previous deeper levels when
leaving a scope. Afterwards, when it entered again a
deeper level, it blindly reused the previous
indentation level. In case the --lines option was used
such that the previous deeper level was not formatted,
that previous level was whatever happened to be there
in the source code. The formatter simply believed it.

This is fixed by letting the LevelIndentTracker forget
the previous deeper levels when stepping out of them
(=> change in LevelIndentTracker::nextLine()).
Note that this used to be the case until LLVM 14.0.6,
but was changed in https://reviews.llvm.org/D129064
(#56352) to fix a crash. Our commit here essentially
reverts that crash fix. It was incorrect/incomplete.

Even with the revert, the crash from
https://reviews.llvm.org/D129064 (#56352) no longer
occurs, most likely because of the changes made in
https://reviews.llvm.org/D144296 (#60843).

The change in adjustToUnmodifiedLine() ensures that
the assert() is only checked if IndentForLevel is
actually accessed.

The new test FormatTestSelective.DontAssert checks
a case where a previous iteration of the present
patch triggered an assert().

The new tests in
FormatMacroRegardlessOfPreviousIndent check the
formatting of a preprocessor #define. They ensure
that the content of LevelIndentTracker::IndentForLevel
does not affect PP directives.

See
https://github.com/llvm/llvm-project/issues/59178#issuecomment-1542637781
for some more details.

This commit is the 2nd try for
https://reviews.llvm.org/D151047


Herald added a comment.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please 
try to ensure all code changes have a unit test (unless this is an `NFC` or 
refactoring, adding documentation etc..)

Add your unit tests in `clang/unittests/Format` and you can build with `ninja 
FormatTests`.  We recommend using the `verifyFormat(xxx)` format of unit tests 
rather than `EXPECT_EQ` as this will ensure you change is tolerant to random 
whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can 
happen if you are trying to improve the annotation phase to ensure we are 
correctly identifying the type of a token, please add a token annotator test in 
`TokenAnnotatorTest.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

Files:
  llvm/bindings/ocaml/llvm/META.llvm.in
  llvm/bindings/ocaml/llvm/llvm_ocaml.h


Index: llvm/bindings/ocaml/llvm/llvm_ocaml.h
===
--- llvm/bindings/ocaml/llvm/llvm_ocaml.h
+++ llvm/bindings/ocaml/llvm/llvm_ocaml.h
@@ -56,7 +56,6 @@
 #define Use_val(v) ((LLVMUseRef)from_val(v))
 #define BasicBlock_val(v) ((LLVMBasicBlockRef)from_val(v))
 #define MemoryBuffer_val(v) ((LLVMMemoryBufferRef)from_val(v))
-#define PassManager_val(v) ((LLVMPassManagerRef)from_val(v))
 
 /* Convert a C pointer to an OCaml option */
 value ptr_to_option(void *Ptr);
Index: llvm/bindings/ocaml/llvm/META.llvm.in
===
--- llvm/bindings/ocaml/llvm/META.llvm.in
+++ llvm/bindings/ocaml/llvm/META.llvm.in
@@ -37,14 +37,6 @@
 archive(native) = "llvm_executionengine.cmxa"
 )
 
-package "ipo" (
-requires = "llvm"
-version  = "@PACKAGE_VERSION@"
-description = "IPO Transforms for LLVM"
-archive(byte) = "llvm_ipo.cma"
-archive(native) = "llvm_ipo.cmxa"
-)
-
 package "debuginfo" (
 requires = "llvm"
 version = "@PACKAGE_VERSION@"
@@ -61,14 +53,6 @@
 archive(native) = "llvm_irreader.cmxa"
 )
 
-package "scalar_opts" (
-requires = "llvm"
-version = "@PACKAGE_VERSION@"
-description = "Scalar Transforms for LLVM"
-archive(byte) = "llvm_scalar_opts.cma"
-archive(native) = "llvm_scalar_opts.cmxa"
-)
-
 package "transform_utils" (
 requires = "llvm"
 version = "@PACKAGE_VERSION@"
@@ -77,22 +61,6 @@
 archive(native) = "llvm_transform_utils.cmxa"
 )
 
-package "vectorize" (
-requires = "llvm"
-version = "@PACKAGE_VERSION@"
-description = "Vector Transforms for LLVM"
-archive(byte) = "llvm_vectorize.cma"
-archive(native) = "llvm_vectorize.cmxa"
-)
-
-package "passmgr_builder" (
-requires = "llvm"
-version = "@PACKAGE_VERSION@"
-description = "Pass Manager Builder for LLVM"
-archive(byte) = "llvm_passmgr_builder.cma"
-archive(native) = "llvm_passmgr_builder.cmxa"
-)
-
 package "target" (
 requires = "llvm"
 version  = "@PACKAGE_VERSION@"


Index: llvm/bindings/ocaml/llvm/llvm_ocaml.h

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added a comment.

Oh damn, I executed `arc diff --update D151047` on the main branch instead of 
my own local branch... Sorry.
Can I somehow revert that? Or should I simply use the "abandon review" action 
and create a entirely new one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 528547.
Sedeniono added a comment.

Next try at getting the 2nd attempt into review. This is again the complete 
fix, since the previous revision got reverted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -528,6 +528,26 @@
 format("  int a;\n"
"void ff() {}",
11, 0));
+
+  // https://github.com/llvm/llvm-project/issues/59178
+  Style = getMozillaStyle();
+  EXPECT_EQ("int a()\n"
+"{\n"
+"return 0;\n"
+"}\n"
+"int b()\n"
+"{\n"
+"  return 42;\n"
+"}",
+format("int a()\n"
+   "{\n"
+   "return 0;\n"
+   "}\n"
+   "int b()\n"
+   "{\n"
+   "return 42;\n" // Format this line only
+   "}",
+   32, 0));
 }
 
 TEST_F(FormatTestSelective, UnderstandsTabs) {
@@ -617,6 +637,70 @@
  "namespace ns1 { namespace ns2 {\n"
  "}}";
   EXPECT_EQ(Code, format(Code, 0, 0));
+
+  Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+ "#define REF(alias) alias alias_var;\n"
+ "}";
+  EXPECT_EQ(Code, format(Code, 51, 1));
+}
+
+TEST_F(FormatTestSelective, FormatMacroRegardlessOfPreviousIndent) {
+  // clang-format currently does not (or should not) take into account the
+  // indent of previous unformatted lines when formatting a PP directive.
+  // Technically speaking, LevelIndentTracker::IndentForLevel is only for non-PP
+  // lines. So these tests here check that the indent of previous non-PP lines
+  // do not affect the formatting. If this requirement changes, the tests here
+  // need to be adapted.
+  Style = getLLVMStyle();
+
+  Style.IndentPPDirectives = FormatStyle::PPDirectiveIndentStyle::PPDIS_None;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#define some\n" // Formatted line
+"#endif\n"   // That this line is also formatted might be a bug.
+"}};", // Dito: Bug?
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_BeforeHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"  #define some\n" // Formatted line
+" #endif\n"
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_AfterHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#  define some\n" // Formatted line
+"#endif\n" // That this line is also formatted might be a bug.
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -74,6 +74,12 @@
: Line.Level * PPIndentWidth;
   Indent += AdditionalIndent;
 } else {
+  // When going to lower levels, forget previous higher levels so that we
+  // recompute future higher levels. But don't forget them if we enter a PP
+  // directive, since these do not terminate a code block.
+  if (!Line.InPPDirective && Line.Level < IndentForLevel.size())
+IndentForLevel.resize(Line.Level + 1, -1);
+
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
@@ -88,13 +94,13 @@

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono added a comment.

@barannikov88 ok, thanks. I just did that. The latest diff looks right. 
I wondered because phabricator suddenly showed changes in ocaml files, which I 
certainly did not want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 529517.
Sedeniono added a comment.

Reformatted comment as requested.


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

https://reviews.llvm.org/D151047

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -166,11 +166,10 @@
   const AdditionalKeywords &Keywords;
   const unsigned AdditionalIndent;
 
-  /// The indent in characters for each level.
-  /// It remembers the indent of previous lines (that are not PP directives) of
-  /// equal or lower levels. This is used to align formatted lines to the 
indent
-  /// of previous non-formatted lines. Think about the --lines parameter of
-  /// clang-format.
+  /// The indent in characters for each level. It remembers the indent of
+  /// previous lines (that are not PP directives) of equal or lower levels. 
This
+  /// is used to align formatted lines to the indent of previous non-formatted
+  /// lines. Think about the --lines parameter of clang-format.
   SmallVector IndentForLevel;
 
   /// Offset of the current line relative to the indent level.


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -166,11 +166,10 @@
   const AdditionalKeywords &Keywords;
   const unsigned AdditionalIndent;
 
-  /// The indent in characters for each level.
-  /// It remembers the indent of previous lines (that are not PP directives) of
-  /// equal or lower levels. This is used to align formatted lines to the indent
-  /// of previous non-formatted lines. Think about the --lines parameter of
-  /// clang-format.
+  /// The indent in characters for each level. It remembers the indent of
+  /// previous lines (that are not PP directives) of equal or lower levels. This
+  /// is used to align formatted lines to the indent of previous non-formatted
+  /// lines. Think about the --lines parameter of clang-format.
   SmallVector IndentForLevel;
 
   /// Offset of the current line relative to the indent level.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked an inline comment as done.
Sedeniono added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:80
+  // directive, since these do not terminate a code block.
+  if (!Line.InPPDirective && Line.Level < IndentForLevel.size())
+IndentForLevel.resize(Line.Level + 1, -1);

HazardyKnusperkeks wrote:
> Do we need this check? I assume `resize` has a check if it has to grow or 
> shrink, so we would check twice when the size is to be changed and once if 
> not.
The `Line.Level < IndentForLevel.size()` is not necessary because of lines 
63+64. `resize` itself can both grow and shrink the container. 
If `Line.Level >= IndentForLevel.size()` and we omit the `Line.Level < 
IndentForLevel.size()` in line 80, then the `resize()` in line 81 does nothing 
because the container already has the size `Line.Level+1`.

I just added the check in line 80 because I thought it made the intention more 
clear and explicit: Forget indent levels when going to smaller levels.
Still, I can of course remove it again. Should I?



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:169-170
 
   /// The indent in characters for each level.
+  /// It remembers the indent of previous lines (that are not PP directives) of
+  /// equal or lower levels. This is used to align formatted lines to the 
indent

HazardyKnusperkeks wrote:
> And then reformat the comment.
Done.



Comment at: clang/unittests/Format/FormatTestSelective.cpp:663
+"#define some\n" // Formatted line
+"#endif\n"   // That this line is also formatted might be a 
bug.
+"}};", // Dito: Bug?

HazardyKnusperkeks wrote:
> Have you tried to see what is causing this?
Yes: It is caused by the `ContinueFormatting` in 
`UnwrappedLineFormatter::format()`, [see 
here](https://github.com/llvm/llvm-project/blob/8abbc96885c836de6912a5f4327519c09f4e528a/clang/lib/Format/UnwrappedLineFormatter.cpp#L1332C4-L1335).
 If that flag is true, clang-format changes the formatting of lines that are 
not affected by the `--lines` option. It more or less stops this at the next 
right-brace `}`. I am guessing that `#endif` should be handled like a 
right-brace there, to stop the formatting, but I am not sure: The "continue 
formatting" was introduced in 
[this](https://github.com/llvm/llvm-project/commit/a1036e5d081dd800c71bdbdf858908b99eed03a4)
 commit (http://reviews.llvm.org/D14105). I don't fully understand the 
intention there, but I guess it is intended for situations where clang-format 
inserts new `{...}` blocks? So how does this translate to PP directives?

In any case, I think this is an entirely different issue. The main point of 
these tests is that the `#define some` line is correct, i.e. that 
`IndentForLevel` is not used for the PP directives, i.e. that the PP directives 
are not affected by the indent of `void test()`.


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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked an inline comment as done.
Sedeniono added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:169-170
 
   /// The indent in characters for each level.
+  /// It remembers the indent of previous lines (that are not PP directives) of
+  /// equal or lower levels. This is used to align formatted lines to the 
indent

Sedeniono wrote:
> HazardyKnusperkeks wrote:
> > And then reformat the comment.
> Done.
Mh, it says [that it couldn't apply the 
patch](https://reviews.llvm.org/harbormaster/build/367648/). I uploaded a patch 
containing as only change the reformatted comment, via the Web UI ("Update 
Diff" at the top right corner). Should I have uploaded a "squashed" diff, 
containing also ALL changes that I made before (the `resize` etc)? In other 
words, is Phabricator expecting individual patches and it merges and 
accumulates them itself? Or does each new uploaded patch overwrite all previous 
patches?


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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 529630.
Sedeniono added a comment.

Reformatted comment, and submitting it via `arc diff --update` now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -166,11 +166,10 @@
   const AdditionalKeywords &Keywords;
   const unsigned AdditionalIndent;
 
-  /// The indent in characters for each level.
-  /// It remembers the indent of previous lines (that are not PP directives) of
-  /// equal or lower levels. This is used to align formatted lines to the 
indent
-  /// of previous non-formatted lines. Think about the --lines parameter of
-  /// clang-format.
+  /// The indent in characters for each level. It remembers the indent of
+  /// previous lines (that are not PP directives) of equal or lower levels. 
This
+  /// is used to align formatted lines to the indent of previous non-formatted
+  /// lines. Think about the --lines parameter of clang-format.
   SmallVector IndentForLevel;
 
   /// Offset of the current line relative to the indent level.


Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -166,11 +166,10 @@
   const AdditionalKeywords &Keywords;
   const unsigned AdditionalIndent;
 
-  /// The indent in characters for each level.
-  /// It remembers the indent of previous lines (that are not PP directives) of
-  /// equal or lower levels. This is used to align formatted lines to the indent
-  /// of previous non-formatted lines. Think about the --lines parameter of
-  /// clang-format.
+  /// The indent in characters for each level. It remembers the indent of
+  /// previous lines (that are not PP directives) of equal or lower levels. This
+  /// is used to align formatted lines to the indent of previous non-formatted
+  /// lines. Think about the --lines parameter of clang-format.
   SmallVector IndentForLevel;
 
   /// Offset of the current line relative to the indent level.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 529631.
Sedeniono added a comment.

Next try at getting all changes to phabricator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -528,6 +528,26 @@
 format("  int a;\n"
"void ff() {}",
11, 0));
+
+  // https://github.com/llvm/llvm-project/issues/59178
+  Style = getMozillaStyle();
+  EXPECT_EQ("int a()\n"
+"{\n"
+"return 0;\n"
+"}\n"
+"int b()\n"
+"{\n"
+"  return 42;\n"
+"}",
+format("int a()\n"
+   "{\n"
+   "return 0;\n"
+   "}\n"
+   "int b()\n"
+   "{\n"
+   "return 42;\n" // Format this line only
+   "}",
+   32, 0));
 }
 
 TEST_F(FormatTestSelective, UnderstandsTabs) {
@@ -617,6 +637,70 @@
  "namespace ns1 { namespace ns2 {\n"
  "}}";
   EXPECT_EQ(Code, format(Code, 0, 0));
+
+  Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+ "#define REF(alias) alias alias_var;\n"
+ "}";
+  EXPECT_EQ(Code, format(Code, 51, 1));
+}
+
+TEST_F(FormatTestSelective, FormatMacroRegardlessOfPreviousIndent) {
+  // clang-format currently does not (or should not) take into account the
+  // indent of previous unformatted lines when formatting a PP directive.
+  // Technically speaking, LevelIndentTracker::IndentForLevel is only for non-PP
+  // lines. So these tests here check that the indent of previous non-PP lines
+  // do not affect the formatting. If this requirement changes, the tests here
+  // need to be adapted.
+  Style = getLLVMStyle();
+
+  Style.IndentPPDirectives = FormatStyle::PPDirectiveIndentStyle::PPDIS_None;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#define some\n" // Formatted line
+"#endif\n"   // That this line is also formatted might be a bug.
+"}};", // Dito: Bug?
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_BeforeHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"  #define some\n" // Formatted line
+" #endif\n"
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_AfterHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#  define some\n" // Formatted line
+"#endif\n" // That this line is also formatted might be a bug.
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -74,6 +74,12 @@
: Line.Level * PPIndentWidth;
   Indent += AdditionalIndent;
 } else {
+  // When going to lower levels, forget previous higher levels so that we
+  // recompute future higher levels. But don't forget them if we enter a PP
+  // directive, since these do not terminate a code block.
+  if (!Line.InPPDirective && Line.Level < IndentForLevel.size())
+IndentForLevel.resize(Line.Level + 1, -1);
+
   Indent = getIndent(Line.Level);
 }
 if (static_cast(Indent) + Offset >= 0)
@@ -88,13 +94,13 @@
   /// level to the same indent.
   /// Note that \c nextLine must have been

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked an inline comment as done.
Sedeniono added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:169-170
 
   /// The indent in characters for each level.
+  /// It remembers the indent of previous lines (that are not PP directives) of
+  /// equal or lower levels. This is used to align formatted lines to the 
indent

barannikov88 wrote:
> Sedeniono wrote:
> > Sedeniono wrote:
> > > HazardyKnusperkeks wrote:
> > > > And then reformat the comment.
> > > Done.
> > Mh, it says [that it couldn't apply the 
> > patch](https://reviews.llvm.org/harbormaster/build/367648/). I uploaded a 
> > patch containing as only change the reformatted comment, via the Web UI 
> > ("Update Diff" at the top right corner). Should I have uploaded a 
> > "squashed" diff, containing also ALL changes that I made before (the 
> > `resize` etc)? In other words, is Phabricator expecting individual patches 
> > and it merges and accumulates them itself? Or does each new uploaded patch 
> > overwrite all previous patches?
> The latter
Ok, it took me 2 tries, but it seems that I managed to upload the full list of 
changes via `arc diff` now. I had to specify the origin commit explicitly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono updated this revision to Diff 530627.
Sedeniono marked an inline comment as done.
Sedeniono added a comment.

Updated diff: Captured the intention of "going to lower levels" via a debug 
assert in nextLine().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTestSelective.cpp

Index: clang/unittests/Format/FormatTestSelective.cpp
===
--- clang/unittests/Format/FormatTestSelective.cpp
+++ clang/unittests/Format/FormatTestSelective.cpp
@@ -528,6 +528,26 @@
 format("  int a;\n"
"void ff() {}",
11, 0));
+
+  // https://github.com/llvm/llvm-project/issues/59178
+  Style = getMozillaStyle();
+  EXPECT_EQ("int a()\n"
+"{\n"
+"return 0;\n"
+"}\n"
+"int b()\n"
+"{\n"
+"  return 42;\n"
+"}",
+format("int a()\n"
+   "{\n"
+   "return 0;\n"
+   "}\n"
+   "int b()\n"
+   "{\n"
+   "return 42;\n" // Format this line only
+   "}",
+   32, 0));
 }
 
 TEST_F(FormatTestSelective, UnderstandsTabs) {
@@ -617,6 +637,70 @@
  "namespace ns1 { namespace ns2 {\n"
  "}}";
   EXPECT_EQ(Code, format(Code, 0, 0));
+
+  Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Code = "namespace ns {\n"
+ "#define REF(alias) alias alias_var;\n"
+ "}";
+  EXPECT_EQ(Code, format(Code, 51, 1));
+}
+
+TEST_F(FormatTestSelective, FormatMacroRegardlessOfPreviousIndent) {
+  // clang-format currently does not (or should not) take into account the
+  // indent of previous unformatted lines when formatting a PP directive.
+  // Technically speaking, LevelIndentTracker::IndentForLevel is only for non-PP
+  // lines. So these tests here check that the indent of previous non-PP lines
+  // do not affect the formatting. If this requirement changes, the tests here
+  // need to be adapted.
+  Style = getLLVMStyle();
+
+  Style.IndentPPDirectives = FormatStyle::PPDirectiveIndentStyle::PPDIS_None;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#define some\n" // Formatted line
+"#endif\n"   // That this line is also formatted might be a bug.
+"}};", // Dito: Bug?
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_BeforeHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"  #define some\n" // Formatted line
+" #endif\n"
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
+
+  Style.IndentPPDirectives =
+  FormatStyle::PPDirectiveIndentStyle::PPDIS_AfterHash;
+  EXPECT_EQ("  class Foo {\n"
+"void test() {\n"
+"#ifdef 1\n"
+"#  define some\n" // Formatted line
+"#endif\n" // That this line is also formatted might be a bug.
+"}};",
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));
 }
 
 } // end namespace
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -74,6 +74,14 @@
: Line.Level * PPIndentWidth;
   Indent += AdditionalIndent;
 } else {
+  // When going to lower levels, forget previous higher levels so that we
+  // recompute future higher levels. But don't forget them if we enter a PP
+  // directive, since these do not terminate a code block.
+  if (!Line.InPPDirective) {
+assert(Line.Level <= IndentForLevel.size());
+IndentForLevel.resize(Line.Level + 1, -1);
+  }
+
   Indent = getIndent(Line.Level);
 }
 if (static_ca

[PATCH] D151047: [clang-format] Fix indentation for selective formatting.

[Sedenion via Phabricator via cfe-commits]

Sedeniono marked an inline comment as done.
Sedeniono added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:80
+  // directive, since these do not terminate a code block.
+  if (!Line.InPPDirective && Line.Level < IndentForLevel.size())
+IndentForLevel.resize(Line.Level + 1, -1);

HazardyKnusperkeks wrote:
> Sedeniono wrote:
> > HazardyKnusperkeks wrote:
> > > Do we need this check? I assume `resize` has a check if it has to grow or 
> > > shrink, so we would check twice when the size is to be changed and once 
> > > if not.
> > The `Line.Level < IndentForLevel.size()` is not necessary because of lines 
> > 63+64. `resize` itself can both grow and shrink the container. 
> > If `Line.Level >= IndentForLevel.size()` and we omit the `Line.Level < 
> > IndentForLevel.size()` in line 80, then the `resize()` in line 81 does 
> > nothing because the container already has the size `Line.Level+1`.
> > 
> > I just added the check in line 80 because I thought it made the intention 
> > more clear and explicit: Forget indent levels when going to smaller levels.
> > Still, I can of course remove it again. Should I?
> You could put the intention in a comment and add an assert.
Ok, I put the check into an `assert()` instead and uploaded a new diff. Note 
that asserting for `Line.Level < IndentForLevel.size()` would be wrong, since 
it can also be equal at this point. The comment already states the intention, 
so did not change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151047

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