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

[Gerhard Gappmeier via Phabricator via cfe-commits]

gergap added inline comments.


================
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:
> > 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   }
```


================
Comment at: clang/unittests/Format/FormatTest.cpp:15068
                    Alignment));
+  EXPECT_EQ("void SomeFunction(int parameter = 0) {\n"
+            "  int const i   = 1;\n"
----------------
curdeius wrote:
> Is there a reason why we don't use `verifyFormat` in these tests?
same answer as above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103245/new/

https://reviews.llvm.org/D103245