[PATCH] D116513: [clang-tidy] Fix bugs in misc-unused-parameters for Constructors calls site

[Aaron Ballman via Phabricator via cfe-commits]

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