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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
