[PATCH] D103245: [clang-format] Fix PointerAlignmentRight with AlignConsecutiveDeclarations

[Björn Schäpers via Phabricator via cfe-commits]

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