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

[Marek Kurdej via Phabricator via cfe-commits]

curdeius added inline comments.


================
Comment at: clang/unittests/Format/FormatTest.cpp:14921
+  verifyFormat("unsigned int       *a;\n"
+               "int                *b;\n"
                "unsigned int Const *c;\n"
----------------
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.


================
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"
----------------
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?


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103245