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

[Owen Pan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0570cc568f5c: [clang-format] Fix indent for selective 
formatting (authored by Sedeniono, committed by owenpan).

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] D151047: [clang-format] Fix indent for selective formatting.

[Owen Pan via Phabricator via cfe-commits]

owenpan 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

Sedeniono wrote:
> 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.
Got it! I didn’t know the assertion was triggered by formatting the closing 
brace. We should keep the test as is then.


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 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 indent for selective formatting.

[Owen Pan via Phabricator via cfe-commits]

owenpan accepted this revision.
owenpan added a comment.

LGTM except for D151047#4508359 .


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.

[Owen Pan via Phabricator via cfe-commits]

owenpan 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

Then I'd remove `Style.FixNamespaceComments` as a factor by formatting a 
non-brace line.


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] 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.

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/unittests/Format/FormatTestSelective.cpp:657
+  Style = getLLVMStyle();
+
+  Style.IndentPPDirectives = FormatStyle::PPDirectiveIndentStyle::PPDIS_None;





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

Use `EXPECT_EQ` to make the default explicit.



Comment at: clang/unittests/Format/FormatTestSelective.cpp:665-671
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));





Comment at: clang/unittests/Format/FormatTestSelective.cpp:681-687
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));





Comment at: clang/unittests/Format/FormatTestSelective.cpp:697-703
+format("  class Foo {\n"
+   "void test() {\n"
+   "#ifdef 1\n"
+   "#define some\n" // format this line
+   " #endif\n"
+   "}};",
+   75, 0));





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

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?


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 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 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)
___

[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] D151047: [clang-format] Fix indent for selective formatting.

[Owen Pan via Phabricator via cfe-commits]

owenpan 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));

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?



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

Again, these tests would pass without the patch.



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

It's the default and can be deleted.



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"

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