HazardyKnusperkeks added a comment.
Please just check `continue`, I would like to make it a separate commit,
because it seems unrelated to me. Otherwise this is good.
================
Comment at: clang/lib/Format/WhitespaceManager.cpp:369
assert(Shift >= 0);
+ if (Shift == 0)
+ continue;
----------------
gergap wrote:
> HazardyKnusperkeks wrote:
> > This is unrelated, isn't it?
> >
> > If it is, I would like a separate commit. Otherwise an explanation why it
> > is now mandatory and does not infer with the other alignments.
> I actually don't know. Just copied this from the original patch.
Could you please check if it works without this?
================
Comment at: clang/unittests/Format/FormatTest.cpp:15045
+ // PAS_RIGHT
EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
" int const i = 1;\n"
----------------
gergap wrote:
> gergap wrote:
> > gergap wrote:
> > > HazardyKnusperkeks wrote:
> > > > I don't know why this is `EXPECT_EQ` instead of `verifyFormat`, but I
> > > > know someone who will request that. :)
> > > >
> > > > You should change that here and use that for your following tests.
> > > I don't know, because I'm not the author of that code.
> > > But I can change it to verifyFormat if this is what you prefer.
> > >
> > The verifyFormat() call with one code parameter does not work with this
> > test pattern, because the internal messUpCode function removes the newlines.
> > The consecutive alignments are interrupted by newlines, which lead to
> > different indent for each section. This breaks with messUpCode.
> >
> > However, there is a verifyFormat function with two code arguments, wich
> > behaves similar to the existing EXPECT_EQ. This way it works.
> > I go for this option now.
> Unfortunately, verifyFormat does not work with this test pattern.
> As you can see in line 80 below there is another mussUp(Code) call for the
> Objective-C++ checks.
> This again screws up the newlines and it does not help to use the
> verifyFormat overload with two code arguments.
>
> I think this is a general limitation. Clang-format distinguishes between
> ACS_Consecutive and ACS_AcrossEmptyLines.
> Using verifyFormat you cannot use empty lines, so you cannot test if
> ACS_Consecutive does NOT align across empty lines.
>
> IMO, it is better to keep the EXPECT_EQ tests. The other solution would be to
> remove the empty lines from the test patterns.
>
> ```
> 68 void _verifyFormat(const char *File, int Line, llvm::StringRef
> Expected,
> 69 llvm::StringRef Code,
> 70 const FormatStyle &Style = getLLVMStyle()) {
> 71 ScopedTrace t(File, Line, ::testing::Message() << Code.str());
> 72 EXPECT_EQ(Expected.str(), format(Expected, Style))
> 73 << "Expected code is not stable";
> 74 EXPECT_EQ(Expected.str(), format(Code, Style));
> 75 if (Style.Language == FormatStyle::LK_Cpp) {
> 76 // Objective-C++ is a superset of C++, so everything checked for
> C++
> 77 // needs to be checked for Objective-C++ as well.
> 78 FormatStyle ObjCStyle = Style;
> 79 ObjCStyle.Language = FormatStyle::LK_ObjC;
> 80 EXPECT_EQ(Expected.str(), format(test::messUp(Code), ObjCStyle));
> 81 }
> 82 }
> ```
A, now I see! The empty line between the blocks. Yeah, no verifyFormat for that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103245/new/
https://reviews.llvm.org/D103245
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
