mehdi_amini added a comment.
Thanks @aaron.ballman for the review!
================
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
----------------
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?)
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
