Applicability of -Wunused-param

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Applicability of -Wunused-param

Yvan Roux via cfe-dev
Many projects, including LLVM have to turn -Wunused-param off, as it has too many false positives.

Most false positives stem from the fact that often a function has to take a parameter not because it wants to, but because:

1. It’s part of an interface, and has to be API/ABI compatible
2. It overrides another function which does need that parameter

 However, this warning is still very useful when the function is:

 - static
 - private
 - in an anonymous namespace

This asks for a warning on unused parameters which is restricted to “private" (static local / etc) functions.
Am I missing something there?
Has anyone tried to implement such a warning before?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Applicability of -Wunused-param

Yvan Roux via cfe-dev
On 18/09/2018 03:40, George Karpenkov via cfe-dev wrote:

> Many projects, including LLVM have to turn -Wunused-param off, as it has too many false positives.
>
> Most false positives stem from the fact that often a function has to take a parameter not because it wants to, but because:
>
> 1. It’s part of an interface, and has to be API/ABI compatible
> 2. It overrides another function which does need that parameter
>
>   However, this warning is still very useful when the function is:
>
>   - static
>   - private
>   - in an anonymous namespace
>
> This asks for a warning on unused parameters which is restricted to “private" (static local / etc) functions.

But the criteria 1/2 whether a function needs to have unused parameters
are orthogonal to whether the function is "private" (per your vague
notion of what that would mean).  For example, a pointer to a static
function can be passed to an API that expects a certain function type.

(At least for C++, an approach to suppress false -Wunused-param is to
leave the parameters that are unused per criteria 1/2 unnamed in the
function definition.  But which of course doesn't catch cases where the
parameter has been left unnamed "maliciously" and a -Wunused-param would
be warranted.)
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Applicability of -Wunused-param

Yvan Roux via cfe-dev
In reply to this post by Yvan Roux via cfe-dev
It’s worth noting that this warning is very easy to silence and it’s probably better to do that in most cases.  There are two common ways of silencing the warning.

Starting with:

```
int foo(int a, int b)
```

You can (in all languages) do this:

```
(void)b;
```

To indicate that `b` is unused.  Most projects have an `UNUSED` macro to do this.  This improves code readability by indicating that the parameter is intentionally unused in this implementation of the function.  C++ provides a nicer way of doing this and you can just rewrite the function declaration to:

```
int foo(int a, int)
```

Now the second parameter exists, but has no name and so is implicitly unused because nothing can possibly refer to it.  This is even easier to read because you know from the first line of the function that this parameter will not be used in the definition.

I believe that clang-tidy can perform the latter transformation automatically, though it’s generally a good idea to audit the non-uses of parameters to check that they’re intentional and unavoidable.

David 

On 18 Sep 2018, at 02:40, George Karpenkov via cfe-dev <[hidden email]> wrote:

Many projects, including LLVM have to turn -Wunused-param off, as it has too many false positives.

Most false positives stem from the fact that often a function has to take a parameter not because it wants to, but because:

1. It’s part of an interface, and has to be API/ABI compatible
2. It overrides another function which does need that parameter

However, this warning is still very useful when the function is:

- static 
- private
- in an anonymous namespace

This asks for a warning on unused parameters which is restricted to “private" (static local / etc) functions.
Am I missing something there?
Has anyone tried to implement such a warning before?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Applicability of -Wunused-param

Yvan Roux via cfe-dev


On Sep 18, 2018, at 12:40 AM, David Chisnall <[hidden email]> wrote:

It’s worth noting that this warning is very easy to silence and it’s probably better to do that in most cases.

Sure, but projects don’t (even Clang and LLVM).

 There are two common ways of silencing the warning.

Starting with:

```
int foo(int a, int b)
```

You can (in all languages) do this:

```
(void)b;
```

To indicate that `b` is unused.  Most projects have an `UNUSED` macro to do this.  This improves code readability by indicating that the parameter is intentionally unused in this implementation of the function.  C++ provides a nicer way of doing this and you can just rewrite the function declaration to:

```
int foo(int a, int)
```

Now the second parameter exists, but has no name and so is implicitly unused because nothing can possibly refer to it.  This is even easier to read because you know from the first line of the function that this parameter will not be used in the definition.

I believe that clang-tidy can perform the latter transformation automatically, though it’s generally a good idea to audit the non-uses of parameters to check that they’re intentional and unavoidable.

David 

On 18 Sep 2018, at 02:40, George Karpenkov via cfe-dev <[hidden email]> wrote:

Many projects, including LLVM have to turn -Wunused-param off, as it has too many false positives.

Most false positives stem from the fact that often a function has to take a parameter not because it wants to, but because:

1. It’s part of an interface, and has to be API/ABI compatible
2. It overrides another function which does need that parameter

However, this warning is still very useful when the function is:

- static 
- private
- in an anonymous namespace

This asks for a warning on unused parameters which is restricted to “private" (static local / etc) functions.
Am I missing something there?
Has anyone tried to implement such a warning before?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Applicability of -Wunused-param

Yvan Roux via cfe-dev
In reply to this post by Yvan Roux via cfe-dev



On 9/18/18 12:40 AM, David Chisnall via cfe-dev wrote:
It’s worth noting that this warning is very easy to silence and it’s probably better to do that in most cases.  There are two common ways of silencing the warning.

Starting with:

```
int foo(int a, int b)
```

You can (in all languages) do this:

```
(void)b;
```

To indicate that `b` is unused.  Most projects have an `UNUSED` macro to do this.  This improves code readability by indicating that the parameter is intentionally unused in this implementation of the function.  C++ provides a nicer way of doing this and you can just rewrite the function declaration to:

```
int foo(int a, int)
```

Now the second parameter exists, but has no name and so is implicitly unused because nothing can possibly refer to it.  This is even easier to read because you know from the first line of the function that this parameter will not be used in the definition.

I believe that clang-tidy can perform the latter transformation automatically, though it’s generally a good idea to audit the non-uses of parameters to check that they’re intentional and unavoidable.

Might be a good idea to add a fix-it note to the diagnostic to do this too. I think that would stop a lot of users from disabling it.

David 

On 18 Sep 2018, at 02:40, George Karpenkov via cfe-dev <[hidden email]> wrote:

Many projects, including LLVM have to turn -Wunused-param off, as it has too many false positives.

Most false positives stem from the fact that often a function has to take a parameter not because it wants to, but because:

1. It’s part of an interface, and has to be API/ABI compatible
2. It overrides another function which does need that parameter

However, this warning is still very useful when the function is:

- static 
- private
- in an anonymous namespace

This asks for a warning on unused parameters which is restricted to “private" (static local / etc) functions.
Am I missing something there?
Has anyone tried to implement such a warning before?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Applicability of -Wunused-param

Yvan Roux via cfe-dev
In reply to this post by Yvan Roux via cfe-dev


On Sep 18, 2018, at 12:40 AM, David Chisnall <[hidden email]> wrote:

It’s worth noting that this warning is very easy to silence and it’s probably better to do that in most cases.  There are two common ways of silencing the warning.

1. There’s actually three ways: one can also do __attribute__((unused))

2. All three ways are bad for some use cases.
As a data point, I have recently tried compiling the static analyzer core with -Wunused-parameter,
and have removed 100+ lines of dead code, so the warning is clearly useful.
I really wish there was a way to enable it in a not-so-disruptive way.

One pattern for which no good suppression work is a method in an abstract base class with a dummy implementation:

e.g. “void foo(A a, B b, C c) {}”

Then all three suppression ways are bad:

 - Adding "(void) p” for each parameter adds unnecessary code to the method body, hiding the fact that this is a dummy do-nothing implementation
 - __attribute__((unused)) makes declaration much harder to read, and hides the meaning
 - Removing the parameter name makes the documentation unusable

George


Starting with:

```
int foo(int a, int b)
```

You can (in all languages) do this:

```
(void)b;
```

To indicate that `b` is unused.  Most projects have an `UNUSED` macro to do this.  This improves code readability by indicating that the parameter is intentionally unused in this implementation of the function.  C++ provides a nicer way of doing this and you can just rewrite the function declaration to:

```
int foo(int a, int)
```

Now the second parameter exists, but has no name and so is implicitly unused because nothing can possibly refer to it.  This is even easier to read because you know from the first line of the function that this parameter will not be used in the definition.

I believe that clang-tidy can perform the latter transformation automatically, though it’s generally a good idea to audit the non-uses of parameters to check that they’re intentional and unavoidable.

David 

On 18 Sep 2018, at 02:40, George Karpenkov via cfe-dev <[hidden email]> wrote:

Many projects, including LLVM have to turn -Wunused-param off, as it has too many false positives.

Most false positives stem from the fact that often a function has to take a parameter not because it wants to, but because:

1. It’s part of an interface, and has to be API/ABI compatible
2. It overrides another function which does need that parameter

However, this warning is still very useful when the function is:

- static 
- private
- in an anonymous namespace

This asks for a warning on unused parameters which is restricted to “private" (static local / etc) functions.
Am I missing something there?
Has anyone tried to implement such a warning before?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Applicability of -Wunused-param

Yvan Roux via cfe-dev
On Mon, 1 Oct 2018 at 16:16, George Karpenkov via cfe-dev <[hidden email]> wrote:


On Sep 18, 2018, at 12:40 AM, David Chisnall <[hidden email]> wrote:

It’s worth noting that this warning is very easy to silence and it’s probably better to do that in most cases.  There are two common ways of silencing the warning.

1. There’s actually three ways: one can also do __attribute__((unused))

Since C++17, there's four ways: you can also use the standard [[maybe_unused]] attribute. It makes sense for us to choose a direction that's aligned with that (doubly so if C adopts that attribute if/when it picks up C++ attribute syntax).

2. All three ways are bad for some use cases.
As a data point, I have recently tried compiling the static analyzer core with -Wunused-parameter,
and have removed 100+ lines of dead code, so the warning is clearly useful.
I really wish there was a way to enable it in a not-so-disruptive way.

One pattern for which no good suppression work is a method in an abstract base class with a dummy implementation:

e.g. “void foo(A a, B b, C c) {}”

Then all three suppression ways are bad:

 - Adding "(void) p” for each parameter adds unnecessary code to the method body, hiding the fact that this is a dummy do-nothing implementation
 - __attribute__((unused)) makes declaration much harder to read, and hides the meaning
 - Removing the parameter name makes the documentation unusable

George


Starting with:

```
int foo(int a, int b)
```

You can (in all languages) do this:

```
(void)b;
```

To indicate that `b` is unused.  Most projects have an `UNUSED` macro to do this.  This improves code readability by indicating that the parameter is intentionally unused in this implementation of the function.  C++ provides a nicer way of doing this and you can just rewrite the function declaration to:

```
int foo(int a, int)
```

Now the second parameter exists, but has no name and so is implicitly unused because nothing can possibly refer to it.  This is even easier to read because you know from the first line of the function that this parameter will not be used in the definition.

I believe that clang-tidy can perform the latter transformation automatically, though it’s generally a good idea to audit the non-uses of parameters to check that they’re intentional and unavoidable.

David 

On 18 Sep 2018, at 02:40, George Karpenkov via cfe-dev <[hidden email]> wrote:

Many projects, including LLVM have to turn -Wunused-param off, as it has too many false positives.

Most false positives stem from the fact that often a function has to take a parameter not because it wants to, but because:

1. It’s part of an interface, and has to be API/ABI compatible
2. It overrides another function which does need that parameter

However, this warning is still very useful when the function is:

- static 
- private
- in an anonymous namespace

This asks for a warning on unused parameters which is restricted to “private" (static local / etc) functions.
Am I missing something there?
Has anyone tried to implement such a warning before?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev