This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add code action to generate a constructor for a C++ class
ClosedPublic

Authored by sammccall on Jan 2 2022, 6:58 PM.

Diff Detail

Event Timeline

sammccall created this revision.Jan 2 2022, 6:58 PM
sammccall requested review of this revision.Jan 2 2022, 6:58 PM

Shouldn't the constructors that need std::move also ensure the <utility> header is also included, or at least transitively included. We don't want to generate constructors that wont compile.

nridge added a subscriber: nridge.Jan 12 2022, 8:54 PM
njames93 added inline comments.Jan 16 2022, 12:27 AM
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp
204

If C is default constructible, would it be nice to skip here instead of failing?

212

Some codebases may not want these to be explicit, would it be wiser to use config to control this?

avogelsgesang added inline comments.Jan 18 2022, 5:58 AM
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp
27–29
// e.g. given `struct S{ int x; unique_ptr<double> y; };`, produces:
//   struct S {
//     int x; unique_ptr<double> y;
//     S(int x, unique_ptr<double> y) : x(x), y(std::move(y)) {}
//   };

or just

// e.g. given `struct S{ int x; unique_ptr<double> y; };`, produces:
//     S(int x, unique_ptr<double> y) : x(x), y(std::move(y)) {}

The tweak does not remove the members, as currently suggested by the comment

50

do we also need to exclude anonymous class declarations here? (Not sure if those are also modelled as CXXRecordDecl in the clang AST...)

134

Is this visitor able to look through using MyType = int; and typedef?
I think we should also add a test case

njames93 added inline comments.Jan 18 2022, 6:35 AM
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp
27–29

That's just a bad comment, the tweak won't remove the members

50

Good point, should also ensure there is a test case for this as well.

avogelsgesang added inline comments.Jan 18 2022, 9:33 AM
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp
50

On 2nd thought: To trigger this tweak, I click on the class name, and since anonymous structs don't have class names, I think the tweak can't even be triggered.

Still probably worth a check here, just to be sure. Or at least an assert+ comment in the code

avogelsgesang added inline comments.Jan 18 2022, 9:38 AM
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp
27–29

I know. Let's fix the comment :)

avogelsgesang added inline comments.Jan 23 2022, 3:25 AM
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp
39

Define constructor (first letter uppercase)
At least this seems to be the convention for most other tweaks.

avogelsgesang added a comment.EditedFeb 10 2022, 10:04 AM

Can we add this tweak as the default "fix-it" action for

Class '...' does not declare any constructor to initialize its non-modifiable members

on structs like

struct X {
   int& a;
}

?

sammccall marked 10 inline comments as done.

address comments
(sorry for long turnaround)

Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 11:25 AM

Can we add this tweak as the default "fix-it" action for

Class '...' does not declare any constructor to initialize its non-modifiable members

on structs like

struct X {
   int& a;
}

?

Not easy to do in this patch, we don't have a good way to associate fixes with clang diagnostics yet.
It's a useful idea but needs a little design I guess.
(-Wswitch kind of works by always marking the tweak as a "fix", which semantically doesn't work here)

clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp
50

Good catch, it's also possible to trigger the code action on the class's braces.
Excluded and added test.

134

We call getCanonicalType() before visiting.

204

Yeah, if it's default constructible but *not movable* then it's probably reasonable.

(The default constructor should be public, but actually this applies to all constructors, so I added this condition everywhere)

212

I'm not sure that adding an option solves a problem, so I'd rather not add one yet.

In most cases, the programmer should be deciding whether the constructor is explicit or not on a case-by-case basis. Having a configurable adds complexity without relieving significant burden.

We do have to have some default, and explicit for single-arg constructors is IMO a better default (recommended by cppcoreguidelines, google style guide)

avogelsgesang accepted this revision.Apr 1 2022, 2:00 PM

LGTM (for whatever that's worth - still pretty unexperienced with the clang AST)

Thanks for picking this up again! Looking forward to have this available in clangd! :)

This revision is now accepted and ready to land.Apr 1 2022, 2:00 PM

Not easy to do in this patch, we don't have a good way to associate fixes with clang diagnostics yet.

Is this a clangd-specific implementation issue or a general short-coming in the LSP protocol?

Thanks!

Not easy to do in this patch, we don't have a good way to associate fixes with clang diagnostics yet.

Is this a clangd-specific implementation issue or a general short-coming in the LSP protocol?

It's a clangd thing, tweaks and diagnostics just don't know about each other at all (diagnostics come fully-formed out of the clang internals, tweaks are an isolated traversal of the AST (possibly a different version).

We can't really change the way the diagnostics are generated, so we'd have to store them and do some pattern matching.

(Clangd does have many fixes that are tightly integrated with diagnostics, these are implemented inside clang. Something as big as generating a constructor is out of scope for clang)

Shouldn't the constructors that need std::move also ensure the <utility> header is also included, or at least transitively included. We don't want to generate constructors that wont compile.

I've added this just as a FIXME for now, because we're missing "framework" support for adding includes and composing separate edits in tweaks.
I think this is probably only a small obstacle in practice, it seems pretty rare to write std::move and need to add the #include for it to compile.