aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp:157-159
// CHECK-FIXES: C() {}
C(int i) {}
// CHECK-MESSAGES: :[[@LINE-1]]:9: warning
----------------
mehdi_amini wrote:
> aaron.ballman wrote:
> > I think this fix is incorrect and should not be applied; it changes the
> > meaning of the interface from having a converting constructor to having a
> > default constructor. I think that needs to be optional behavior because
> > it's a pretty invasive change to apply automatically. This patch builds on
> > top of the existing incorrect behavior, but would be fine if it was only
> > applied when the option to modify constructors is enabled.
> I'm not against an option, but I'd like to get to a default behavior that is
> "safe". So I guess my first patch should be to undo the transformation
> happening here in the test already.
> We can either never touch any C++ constructor or try to find a conservative
> logic for it.
> I mentioned in the other review that we may avoid touching a Ctor with a
> single parameter.
>
> Then we also can't do it for a Ctor with two parameters as it'll turn it into
> a converting ctor. Unless you can eliminate both parameters, in which case it
> is become a default ctor (which can conflict with an existing one, in which
> case it can be just deleted?)
>
> I'm not against an option, but I'd like to get to a default behavior that is
> "safe".
Definitely agreed!
> So I guess my first patch should be to undo the transformation happening here
> in the test already.
I think that's a good approach.
> We can either never touch any C++ constructor or try to find a conservative
> logic for it.
Initially, I'd say let's not touch any C++ constructors. We may be able to find
conservative logic for it, but that'll take time to get right.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116513/new/
https://reviews.llvm.org/D116513
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
