[clang-tools-extra] [clangd] Fix OverridePureVirtuals to preserve attributes and formatting (PR #184023)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Mar 29 01:08:51 PDT 2026
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
More information about the cfe-commits
mailing list