https://github.com/timon-ul requested changes to this pull request.
Hi @marcogmaia , sorry for the longer wait, but I am more than happy to be your
reviewer! I am still a bit new to this code, but I think we can figure this out
together!
Overall I would like some clarification of the scope of this PR. You are doing
more than just fixing the linked original issue, mainly preserving (some)
comments and changing formatting a bit and I am wondering if the current
behaviour is truly what you envisioned. Let me say that I do not thing the
behaviour is problematic in any way, just the description of your PR makes it
sound very different.
First let's start with comments, I personally have to admit, I do not see much
value in their preservation, at most for the inside of the function argument
list, but even there I am wondering a bit if a user cares much (but maybe you
do, so let me know). That being said the current behaviour is the following:
```
class Base {
// description
/*0*/virtual/*1*/ int /*2*/foo(/*3*/) /*4*/const/*5*/ = 0/*6*/;
};
class D : Base {
private:
/*1*/ int /*2*/ foo(/*3*/) /*4*/ const /*5*/ override {
// TODO: Implement this pure virtual method.
static_assert(false, "Method `foo` is not implemented.");
}
};
```
As you can see, not all comments make it to the override, again I do not think
this matters for a user, but it does seem a little bit inconsistent.
The next thing I want to adress is formatting. You claim you preserve
formatting, but in fact you do not, you are almost as restrictive as the old
code with formatting, just that it is a different formatting. Consider the
following example:
```
class Base {
[[nodiscard]]
virtual
int
foo(int * x)
const
=
0;
};
class D : Base {
private:
[[nodiscard]]
int foo(int *x) const override {
// TODO: Implement this pure virtual method.
static_assert(false, "Method `foo` is not implemented.");
}
};
```
Notably we have a newline too much and all the other newlines are just replaced
with regular whitespace, but also the argument list changes their whitespace
(`x` now is next to the pointer). I think we can agree that the one too many
newlines is a silly issue you should change, but outside of that I am
wondering, what are we preserving here? Only two of the newlines are actually
preserved, so we are not doing a full preservation, but it is more than
nothing, this is a bit odd in between. Again, this is some super niche edge
case most users will probably never run into (or care), but I am wondering if
this is the behaviour you want. At least your comments are not reflecting what
is actually happening.
https://github.com/llvm/llvm-project/pull/184023
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
