aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst:43
+ a human reader, and there's basically no place for a bug to hide. On the
other
+ hand for non-public functions, all the call-sites are visible and the
parameter
+ can be eliminated entirely.
----------------
mehdi_amini wrote:
> aaron.ballman wrote:
> > Call sites are not always visible for protected functions, so this seems a
> > bit suspicious. The changes are missing test coverage for that situation.
> You're using `public` for "access control" while I was using the linkage
> aspect: my reasoning is that if a method isn't "externally visible" from the
> current translation unit, you see all the call-sites. This is orthogonal to
> public/private/protected as far as I know.
>
> I am likely missing a check for "isDefinedInMainFile" (or whatever the api is
> called) to not flag included headers.
> You're using public for "access control" while I was using the linkage
> aspect: my reasoning is that if a method isn't "externally visible" from the
> current translation unit, you see all the call-sites.
Oh, thanks for pointing out that I was confused! I'm not used to "public" when
describing linkage, usually that's "external" or "externally visible". Any
appetite for rewording in those terms? Something like "On the other hand, for
functions with internal linkage, all the call sites are visible and parameters
can be safely removed."
================
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 demonstrates a bad fix -- this changes code meaning from being
> > a converting constructor to being a default constructor, which are very
> > different things.
> Oh you're right: so we can't do it for a Ctor with a single parameter...
>
> But 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?)
Yeah, I think we need to not transform ctors at all currently.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116512/new/
https://reviews.llvm.org/D116512
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
