[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