Bug 68230 - Unused function parameters not reported by -Wunused-parameter when only used recursively (add -Wparameter-only-used-recursively instead?)
Summary: Unused function parameters not reported by -Wunused-parameter when only used ...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 5.2.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: new-warning Wunused
  Show dependency treegraph
 
Reported: 2015-11-06 01:11 UTC by Joshua T, Fisher
Modified: 2020-03-04 07:50 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-09-28 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua T, Fisher 2015-11-06 01:11:35 UTC
The code below does not produce a warning when -Wunused-parameter is turned on. It would be fantastic if it did, as fundamentally, the variable notUsed is still not actually being used.

#include <iostream>
using namespace std;

int Recursive(int num, const char *notUsed);

int main() {
  const char *notUsedLocal = "Not used";

  cout << "Test: " << Recursive(1, notUsedLocal) << endl;
  return 0;
}

int Recursive(int num, const char *notUsed) {
  if (num > 5)
    return num;
  else
    return Recursive(++num, notUsed);
}
Comment 1 Eric Gallager 2017-09-28 12:41:18 UTC
Confirmed that there's currently no warning.
Comment 2 Jonathan Wakely 2017-09-28 13:00:28 UTC
I'm not sure there *should* be a warning here.

I find -Wunused-parameter most useful to know when I can remove the name of the parameter, but you can't do that here (because it still needs a name to pass it to the recursive call).

It's rare that my reaction to a -Wunused-parameter warning is to change the function to remove the parameter entirely. YMMV.
Comment 3 Eric Gallager 2017-09-28 14:38:08 UTC
(In reply to Jonathan Wakely from comment #2)
> I'm not sure there *should* be a warning here.
> 
> I find -Wunused-parameter most useful to know when I can remove the name of
> the parameter, but you can't do that here (because it still needs a name to
> pass it to the recursive call).
> 
> It's rare that my reaction to a -Wunused-parameter warning is to change the
> function to remove the parameter entirely. YMMV.

There's also attribute unused and pragma unused, but yeah, it'd feel weird using either of those here, too... maybe this should be closed as WONTFIX instead...
Comment 4 Jonathan Wakely 2017-09-28 15:09:38 UTC
Those aren't portable, so aren't a good general solution.
Comment 5 Joshua T, Fisher 2017-09-28 17:51:36 UTC
(In reply to Jonathan Wakely from comment #2)
> It's rare that my reaction to a -Wunused-parameter warning is to change the
> function to remove the parameter entirely. YMMV.

Totally reasonable, I too rarely change the signature and use either attributes, removing the parameter name, or the void cast trick. However I have seen bugs pop up a couple times from important parameters being included, but not used, see:

https://github.com/nvpro-pipeline/VkHLF/commit/b6646c4773e8aef49c40e8684eca1382bf2e9d50

and my blog where I first encountered this in a closed source codebase:

http://www.nullterminatedstrings.com/c++/recursive-warning

Perhaps this doesn't qualify as a bug in the current -Wunused-parameter warning implementation, but would it be reasonable to add it as a separate warning?

Thanks for looking into this by the way folks!
Comment 6 Joshua T, Fisher 2017-09-28 21:00:02 UTC
(In reply to Joshua T, Fisher from comment #5)
> https://github.com/nvpro-pipeline/VkHLF/commit/b6646c4773e8aef49c40e8684eca1382bf2e9d50

Apologies this one doesn't apply. Just took another look and I just misremembered, this is just a normal -Wunused-parameter, I don't think they had it turned on.
Comment 7 Jonathan Wakely 2017-09-28 23:57:15 UTC
(In reply to Joshua T, Fisher from comment #5)
> and my blog where I first encountered this in a closed source codebase:
> 
> http://www.nullterminatedstrings.com/c++/recursive-warning

I'm a bit more persuaded having read this. If it points out you forgot to use it (for anything except the recursion) that could help find real bugs.
Comment 8 Joshua T, Fisher 2017-10-03 00:47:42 UTC
(In reply to Jonathan Wakely from comment #7)
> (In reply to Joshua T, Fisher from comment #5)
> > and my blog where I first encountered this in a closed source codebase:
> > 
> > http://www.nullterminatedstrings.com/c++/recursive-warning
> 
> I'm a bit more persuaded having read this. If it points out you forgot to
> use it (for anything except the recursion) that could help find real bugs.

That's what I was hoping! I feel like it's definitely something that could hit some real code bases. I find it unlikely I'm the only person ever to run into this.
Comment 9 Eric Gallager 2018-10-03 02:24:31 UTC
(In reply to Joshua T, Fisher from comment #8)
> (In reply to Jonathan Wakely from comment #7)
> > (In reply to Joshua T, Fisher from comment #5)
> > > and my blog where I first encountered this in a closed source codebase:
> > > 
> > > http://www.nullterminatedstrings.com/c++/recursive-warning
> > 
> > I'm a bit more persuaded having read this. If it points out you forgot to
> > use it (for anything except the recursion) that could help find real bugs.
> 
> That's what I was hoping! I feel like it's definitely something that could
> hit some real code bases. I find it unlikely I'm the only person ever to run
> into this.

Maybe it'd be possible to put it under a separate flag (e.g. -Wparameter-only-used-recursively) so that it'd be easier to focus on just that kind of warning individually? There are some codebases that turn off -Wunused-parameter specifically that still might want this warning.
Comment 10 Eric Gallager 2019-09-19 02:52:32 UTC
Martin Jambor's IPA-SRA rewrite might be relevant here; it sounded like the new IPA-SRA will remove parameters that are unused like this, but I didn't quite catch if it'd also warn about them...
Comment 11 Eric Gallager 2019-12-02 03:14:46 UTC
bug 32562 is related (that's for functions, this is for parameters)
Comment 12 Martin Jambor 2019-12-02 12:54:17 UTC
(In reply to Eric Gallager from comment #10)
> Martin Jambor's IPA-SRA rewrite might be relevant here; it sounded like the
> new IPA-SRA will remove parameters that are unused like this, but I didn't
> quite catch if it'd also warn about them...

IPA-SRA can remove parameters that are only used to be passed to
another function in a chain of recursive calls, it does not even have
to be a direct recursion (but it can).  It does not warn but it could.

One problem is however that IPA-SRA only modifies and analyzes "local"
functions, so in non-LTO mode it only works if all of the recursive
functions were static or in an anonymous C++ namespace.  With LTO,
anything not exported outside the result executable/shared object
would do but you'd only get your warnings at link-time.

Please also note that with this particular testcase, by the time IPA
passes see the function the recursion is converted to tail-call and
since IPA-CP (running just before IPA-SRA) can also remove totally
unused parameters, IPA-SRA does not do anything in this case even with
-fwhole-program.