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