[PATCH] D116514: [clangd] Add code action to generate a constructor for a C++ class

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D116514#3311780 <https://reviews.llvm.org/D116514#3311780>, @avogelsgesang wrote:

> 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)



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:49
+      Class = N->ASTNode.get<CXXRecordDecl>();
+    if (!Class || !Class->isThisDeclarationADefinition() || Class->isUnion())
+      return false;
----------------
avogelsgesang wrote:
> njames93 wrote:
> > avogelsgesang wrote:
> > > do we also need to exclude anonymous class declarations here? (Not sure if those are also modelled as `CXXRecordDecl` in the clang AST...)
> > Good point, should also ensure there is a test case for this as well.
> 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
Good catch, it's also possible to trigger the code action on the class's braces.
Excluded and added test.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:133
+    // Decide what to do based on the field type.
+    class Visitor : public TypeVisitor<Visitor, FieldAction> {
+    public:
----------------
avogelsgesang wrote:
> Is this visitor able to look through `using MyType = int;` and `typedef`?
> I think we should also add a test case
We call getCanonicalType() before visiting.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:203
+      return CopyRef;
+    return Fail;
+  }
----------------
njames93 wrote:
> If C is default constructible, would it be nice to skip here instead of failing?
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)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:211
+    if (Fields.size() == 1)
+      OS << "explicit ";
+    OS << Class->getName() << "(";
----------------
njames93 wrote:
> Some codebases may not want these to be explicit, would it be wiser to use config to control this?
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](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c46-by-default-declare-single-argument-constructors-explicit), [google style guide](https://google.github.io/styleguide/cppguide.html#Implicit_Conversions))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116514