[PATCH] D116512: [clang-tidy] Limit non-Strict mode to public functions

[Mehdi AMINI via Phabricator via cfe-commits]

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:
> 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.


================
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 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?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116512/new/

https://reviews.llvm.org/D116512