[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:14921
+  verifyFormat("unsigned int       *a;\n"
+               "int                *b;\n"
                "unsigned int Const *c;\n"
----------------
curdeius wrote:
> HazardyKnusperkeks wrote:
> > MyDeveloperDay wrote:
> > > I seem to remember in the past there was a comment from @djasper in the past that he felt you shouldn't align the * like this
> > > 
> > > I think it depends on your viewpoint as to whether the * goes with the type or the variable, this could be a religious debate, I don't think you can just assume everyone agrees with this, the fact you are changing unit tests just leaves me with alarm bells going off...
> > > 
> > > I'm not comfortable with you changing unit tests. This implies that you think its a bug, but I think the fact the test exists (Beyonce rule), means someone may have wanted to assert that behaviour. 
> > > 
> > >  Should we have an option to control this?
> > As far as I understand it `PAS_Right` says it should stick with the variable, and thus align this way, if we align declarations.
> > 
> > I think this test was this way because of the `FIXME`.
> I agree with @HazardyKnusperkeks. But I agree that it might be a contentious subject.
The unit tests were wrong IMO and were just hacked that way so that the tests don't fail.
Hence, it was correct to fix the tests, because this feature now works.
Normally such tests should be coded as "expected failure" to make this clear, but this was not the case.

PAS_Right says it should stick with the variable, not with the type.
See https://www.kernel.org/doc/html/v4.10/process/coding-style.html#spaces for an example.


================
Comment at: clang/unittests/Format/FormatTest.cpp:15046-15048
             "  int const i   = 1;\n"
-            "  int *     j   = 2;\n"
+            "  int      *j   = 2;\n"
             "  int       big = 10000;\n"
----------------
curdeius wrote:
> curdeius wrote:
> > HazardyKnusperkeks wrote:
> > > But maybe than this should be aligned as that?
> > Hmm, that's a subjective choice here indeed.
> > We might either 1) choose one or another (I'm slightly in favor of aligning variable names, as it's done currently), or  2) add a config option.
> > For 1), it would be wise to see large projects with a similar style and what they do (and whethere there's some consensus).
> > Anyone aware of such projects?
> That makes me think. How would be this formatted:
> ```
> int **************lotsOfpointers;
> int               i;
> ```
> 
> @gergap, could you add such a test or point me to an existing one that tests the same thing, please?
> That sort of test might fail if pointers/references weren't accounted for in the length of the type name itself.
It's a religious question, like everything related to styles. From my experience, aligning at the variable name (excluding type modifiers) is the common case. It just looks nicer aligned this way.

```
int   a;
int  *b;
int **c;
int   foobar;
```

It also makes more sense from the logical point of view. First we align the variable names then the pointers are placed right most (PAS_Right), left most (PAS_Left) or in the middle (PAS_Middle).
The middle one is a more complicated question, because even if the pointers are not attached to type or variable there is still the question of the pointer alignment. I didn't change anything here and just kept the logic as-is, which means pointers are left-aligned with one space between type and pointer

@curdeius I added a new test for this which I will upload shortly.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103245