mehdi_amini abandoned this revision.
mehdi_amini 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.
----------------
aaron.ballman wrote:
> 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."
Sure: happy to reword.
(We use public/private for symbol visibility at a module level in MLIR
unfortunately, I've been "contaminated" ;) ).
================
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:
> 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.
In this case I can fix the existing bug by disabling the fixit (as discussed
in D116513) and abandon this revision I think.
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
