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
