[Bug c++/50477] New: -Wunused-parameter should not warn about virtual method declarations with bodies

[Bug c++/50477] New: -Wunused-parameter should not warn about virtual method declarations with bodies

[miles at gnu dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50477

             Bug #: 50477
           Summary: -Wunused-parameter should not warn about virtual
                    method declarations with bodies
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: trivial
          Priority: P3
         Component: c++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: miles@gnu.org


In the following code:

   struct X { virtual void f (int a) { } };

One gets a warning from -Wunused-parameter, "unused parameter 'a'".

While this is understandable, I think it's undesirable -- clearly the parameter
'a' will often be used by overrides of X::f, and the parameter name 'a' serves
a valuable documentation purpose here, even if it's not strictly required.

The basic problem of course, is that in this case, f is both a virtual method
declaration which covers overriding methods as well, and a definition of X's
definition of it.

One can avoid this by defining X::f outside the class definition, but for
trivial definitions like the above, this would seem like annoying make-work if
done only to shut up -Wunused-parameter.

My current method of avoiding the warning is to comment out the parameter name: 

   virtual void f (int /* a */) { }

but this again seems like an annoying wart, rather than natural code -- it's
both less readable (especially when the parameter has a funny type like a
function pointer) and yields a slightly confusing inconsistency ("why do
comment-out some method parameter names but not others?!" "oh it's just to shut
up -Wunused-parameter... :(").

Anyway, the current behavior seems wrong to me; I think -Wunused-parameter
shoul d stay silent for method virtual definitions inside the class definition.

[If this were a very rare scenario, I suppose I wouldn't care about the need to
work around it, but it seems to happen all the time for me...]

Thanks,

-Miles

[Bug c++/50477] -Wunused-parameter should not warn about virtual method declarations with bodies

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50477

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> 2011-09-22 21:14:56 UTC ---
Having a name vs having correct documentation is the best way.  Also you can
use the attribute unused in C++ now.

[Bug c++/50477] -Wunused-parameter should not warn about virtual method declarations with bodies

[miles at gnu dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50477

--- Comment #2 from miles at gnu dot org 2011-09-22 22:34:01 UTC ---
(In reply to comment #1)
> Having a name vs having correct documentation is the best way.  Also you can
> use the attribute unused in C++ now.

Sorry, I'm not sure I understand your first sentence.  [Of course these methods
also have proper documentation, but that doesn't mean it's OK for the code to
be obfuscated; _both_ should be readable and clear.]

Yeah, you can use an "unused" attribute (or various other methods), but the
problem is that they all uglify the code.  This would be acceptable if it were
a rare case, or one where adding an unused attribute actually provided useful
information -- such as in a _separate_ method definition, where the fact that
the argument is unused is notable.

However my experience is that this case is not rare at all, and in fact, I find
it very common for the root of a class hierarchy to provide trivial/no-op
default definitions for methods that are given more complicated definitions in
subclasses.

Moreover, adding an "unused" attribute actually seems _harmful_ in this case
(in the sense of "confusing"), because it's not just a definition of a
particular class's method body, it's a method declaration that applies to all
the subclasses too.  So if a user sees "attribute ((unused))" next to an
argument in the declaration, a natural question might be "wait, what? am I not
supposed to use this argument in my subclass definitions??"

[If it were a human, a better criteria than just "virtual method with body
defined in class" might include "... and also an obviously trivial definition"
(e.g., {} or {return true;}, etc); but I'm not sure it's reasonable to ask the
compiler to look at that kind of thing...]

[Bug c++/50477] -Wunused-parameter should not warn about virtual method declarations with bodies

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50477

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-09-23 08:27:09 UTC ---
So just use -Wno-unused-parameter if you want to give the arguments names,
don't use them and don't like unused attribute.  I don't see why inline
methods, virtual or non-virtual, should be any different in this regard from
any other functions.

[Bug c++/50477] -Wunused-parameter should not warn about virtual method declarations with bodies

[miles at gnu dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50477

--- Comment #4 from miles at gnu dot org 2011-09-23 08:59:47 UTC ---
It's different because unlike most function/method definitions, this is _two_
things together -- a virtual method declaration, which pertains to this class
and all subclasses, and a method definition, which only instead only really
pertains to this class.

Note that if you separate the definition and the declaration, there's no issue
-- the definition can have the parameter name omitted, or use the "unused"
attribute, and it makes perfect sense, both to the compiler, and to readers. 
But if one uses the inline definition, there's no way to separate these two
uses; the compiler still understands of course; it's readers who are
inconvenienced, because they see code obfuscation intended for the _definition_
when they read the code to see the _declaration_.

If it were a simple function (or an out-of-line method definition, or a
non-virtual method), I'd happily use attribute "unused" (or whatever), because
it clearly does apply to the definition, and indeed makes the code more
readable by documenting something.

It's only in this case where two different things are mixed together that I'm
uncomfortable doing so.

I'm not denying that there are many ways to make the compiler happy; my
complaint is that they all make the code less readable, and I think readability
is important.

[Bug c++/50477] -Wunused-parameter should not warn about virtual method declarations with bodies

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50477

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org

--- Comment #5 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-05-24 15:33:22 UTC ---
Jason, it would be great if you could help us completing the triage of this.

[Bug c++/50477] -Wunused-parameter should not warn about virtual method declarations with bodies

[jason at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50477

--- Comment #6 from Jason Merrill <jason at gcc dot gnu.org> 2012-05-24 19:46:02 UTC ---
I sympathize with your argument, but I think that some users will want the
current behavior.  One way you could work around this that wouldn't involve
uglifying every affected function would be to add

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-parameter"

....

#pragma GCC diagnostic pop

around the header.

[Bug c++/50477] -Wunused-parameter should not warn about virtual method declarations with bodies

[anthony.foiani at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50477

Anthony Foiani <anthony.foiani at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |anthony.foiani at gmail dot com

--- Comment #7 from Anthony Foiani <anthony.foiani at gmail dot com> ---
Sorry to resurrect an oldish bug, but I was just bitten by a related issue (bug
57211).

One case where I run into Miles' situation is when writing documentation; as
mentioned, root classes often have no-op stubs, but they're also where those
stubs are documented.  E.g.,

    /**
     * Do stuff with @a foo.
     *
     * @param[in]   foo     widget to frobnicate.
     *
     * @return whether foo was successfully frobbed.
     */

    virtual bool frobnicate( Widget & foo ) {}

If I use "/* foo */", then doxygen will complain that there is no matching
parameter name.

If I don't use those comments, then -Wunused-parameters gives me a warning.

Would it be possible to introduce something like -Wno-virtual-unused-parameters
to accomodate this?

Or possibly merge it with a flag that would allow similar ignoring of defaulted
methods (which is how I got here).

Thanks!