[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