[PATCH] D116512: [clang-tidy] Limit non-Strict mode to public functions
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
[PATCH] D116512: [clang-tidy] Limit non-Strict mode to public functions
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
[PATCH] D116512: [clang-tidy] Limit non-Strict mode to public functions
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D116512: [clang-tidy] Limit non-Strict mode to public functions
aaron.ballman requested changes to this revision.
aaron.ballman added inline comments.
This revision now requires changes to proceed.
Comment at: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp:188
// (constructor initializer counts for non-empty body).
-if (StrictMode ||
+if (StrictMode || !Function->isExternallyVisible() ||
(Function->getBody()->child_begin() !=
This is looking at the linkage of the function, not at its access control; is
that intended?
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.
Call sites are not always visible for protected functions, so this seems a bit
suspicious. The changes are missing test coverage for that situation.
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
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.
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
[PATCH] D116512: [clang-tidy] Limit non-Strict mode to public functions
mehdi_amini created this revision.
mehdi_amini added a reviewer: Eugene.Zelenko.
Herald added subscribers: carlosgalvezp, xazax.hun.
mehdi_amini requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
The rational to avoid applying the warning/fix in non-Strict more to
functions with an empty body is that there is "no place for a bug to hide".
However for private functions, the parameters can be entirely eliminated
and all the call sites improved.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D116512
Files:
clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
@@ -154,6 +154,10 @@
namespace {
class C {
public:
+// CHECK-FIXES: C() {}
+ C(int i) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning
+
void f(int i);
// CHECK-FIXES: void f();
void g(int i) {;}
@@ -181,7 +185,7 @@
void useFunction(T t);
void someMoreCallSites() {
- C c;
+ C c(42);
c.f(1);
// CHECK-FIXES: c.f();
c.g(1);
Index: clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
@@ -37,6 +37,8 @@
When `false` (default value), the check will ignore trivially unused
parameters,
i.e. when the corresponding function has an empty body (and in case of
- constructors - no constructor initializers). When the function body is
empty,
- an unused parameter is unlikely to be unnoticed by a human reader, and
- there's basically no place for a bug to hide.
+ constructors - no constructor initializers) and the definition is public.
When
+ the function body is empty, an unused parameter is unlikely to be unnoticed
by
+ 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.
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -183,9 +183,9 @@
Param->hasAttr())
continue;
-// In non-strict mode ignore function definitions with empty bodies
+// In non-strict mode ignore public function definitions with empty bodies
// (constructor initializer counts for non-empty body).
-if (StrictMode ||
+if (StrictMode || !Function->isExternallyVisible() ||
(Function->getBody()->child_begin() !=
Function->getBody()->child_end()) ||
(isa(Function) &&
Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
@@ -154,6 +154,10 @@
namespace {
class C {
public:
+// CHECK-FIXES: C() {}
+ C(int i) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning
+
void f(int i);
// CHECK-FIXES: void f();
void g(int i) {;}
@@ -181,7 +185,7 @@
void useFunction(T t);
void someMoreCallSites() {
- C c;
+ C c(42);
c.f(1);
// CHECK-FIXES: c.f();
c.g(1);
Index: clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-unused-parameters.rst
@@ -37,6 +37,8 @@
When `false` (default value), the check will ignore trivially unused parameters,
i.e. when the corresponding function has an empty body (and in case of
- constructors - no constructor initializers). When the function body is empty,
- an unused parameter is unlikely to be unnoticed by a human reader, and
- there's basically no place for a bug to hide.
+ constructors - no constructor initializers) and the definition is public. When
+ the function body is empty, an unused parameter is unlikely to be unnoticed by
+ 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.
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
