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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
