[Bug bootstrap/91972] Bootstrap should use -Wmissing-declarations

[Bug bootstrap/91972] Bootstrap should use -Wmissing-declarations

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91972

--- Comment #1 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Another reason to have -Wmissing-declarations is that otherwise mismatches of
unused functions are not caught until it's too late (mismatching definition is
assumed to be an overload of the function declared in the header file).

For a recent example, see
https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545129.html which was
necessary after a mismatch introduced in
https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545114.html

[Bug bootstrap/91972] Bootstrap should use -Wmissing-declarations

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91972

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Alexander Monakov from comment #0)
> Transition to C++ did not change -Wmissing-prototypes to
> -Wmissing-declarations, so over time several violations crept in. In
> particular this penalizes optimization during non-LTO bootstrap (the
> compiler has to assume the function might be used in another TU, even though
> in reality all uses are in current file and it simply misses the 'static'
> keyword).

Why is it missing the static keyword then? (Or alternatively, why isn't it in
an anonymous namespace?)


(In reply to Alexander Monakov from comment #1)
> Another reason to have -Wmissing-declarations is that otherwise mismatches
> of unused functions are not caught until it's too late (mismatching
> definition is assumed to be an overload of the function declared in the
> header file).

A more robust way to avoid that problem is to declare the function in a
namespace, and define it using a qualified name:

// declaration
namespace targ
{
  void foo(void*);
}

// definition
void targ::foo(class vec_info*);  // ERROR

Because no foo with that signature was declared in namespace targ it's an
error, not just a warning.

Should the coding convention be adjusted to avoid this problem?

[Bug bootstrap/91972] Bootstrap should use -Wmissing-declarations

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91972

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #2)
> (In reply to Alexander Monakov from comment #0)
> > Transition to C++ did not change -Wmissing-prototypes to
> > -Wmissing-declarations, so over time several violations crept in. In
> > particular this penalizes optimization during non-LTO bootstrap (the
> > compiler has to assume the function might be used in another TU, even though
> > in reality all uses are in current file and it simply misses the 'static'
> > keyword).
> 
> Why is it missing the static keyword then? (Or alternatively, why isn't it
> in an anonymous namespace?)
> 
> 
> (In reply to Alexander Monakov from comment #1)
> > Another reason to have -Wmissing-declarations is that otherwise mismatches
> > of unused functions are not caught until it's too late (mismatching
> > definition is assumed to be an overload of the function declared in the
> > header file).
> 
> A more robust way to avoid that problem is to declare the function in a
> namespace, and define it using a qualified name:
> 
> // declaration
> namespace targ
> {
>   void foo(void*);
> }
> 
> // definition
> void targ::foo(class vec_info*);  // ERROR
> 
> Because no foo with that signature was declared in namespace targ it's an
> error, not just a warning.
> 
> Should the coding convention be adjusted to avoid this problem?

Ah, I like the namespace thing for target hooks (possibly langhooks as well).

[Bug bootstrap/91972] Bootstrap should use -Wmissing-declarations

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91972

--- Comment #4 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
> Why is it missing the static keyword then? (Or alternatively, why isn't it in an anonymous namespace?)

Huh? Without the warning developers may simply forget to put the 'static'
keyword. With the warning they would be reminded when bootstrapping the patch.


> Ah, I like the namespace thing for target hooks (possibly langhooks as well).

Sure, it's nice to have sensible namespace rules for future additions, but
hopefully that's not a reason/excuse to never re-enable the warning.

[Bug bootstrap/91972] Bootstrap should use -Wmissing-declarations

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91972

--- Comment #5 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to Alexander Monakov from comment #4)
> > Why is it missing the static keyword then? (Or alternatively, why isn't it in an anonymous namespace?)
> 
> Huh? Without the warning developers may simply forget to put the 'static'
> keyword. With the warning they would be reminded when bootstrapping the
> patch.
> 
> 
> > Ah, I like the namespace thing for target hooks (possibly langhooks as well).
> 
> Sure, it's nice to have sensible namespace rules for future additions, but
> hopefully that's not a reason/excuse to never re-enable the warning.

Agreed; I think I tried enabling the warning once while bootstrapping myself,
but I forget what the results were...

[Bug bootstrap/91972] Bootstrap should use -Wmissing-declarations

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91972

--- Comment #6 from Eric Gallager <egallager at gcc dot gnu.org> ---
To clarify, are we talking about adding it just in the gcc subdirectory, or any
other subdirectories as well?

[Bug bootstrap/91972] Bootstrap should use -Wmissing-declarations

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91972

--- Comment #7 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
As I understand, only the gcc subdirectory changed implementation language from
C to C++, so, yes (as far as this bug is concerned).

[Bug bootstrap/91972] Bootstrap should use -Wmissing-declarations

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91972

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |easyhack

--- Comment #8 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to Alexander Monakov from comment #7)
> As I understand, only the gcc subdirectory changed implementation language
> from C to C++, so, yes (as far as this bug is concerned).

ok, that makes things simpler; adding the "easyhack" keyword

(I might take a shot at this myself, but will stop short of self-assigning,
since there's other things I'm focusing on currently)

[Bug bootstrap/91972] Bootstrap should use -Wmissing-declarations

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91972

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|easyhack                    |

--- Comment #9 from Eric Gallager <egallager at gcc dot gnu.org> ---
so, I've tried enabling this flag while building, and one problem with it is
that GCC's genfoo machinery for its garbage collection system creates a bunch
of gt-*.h headers with a bunch of autogenerated gt_ggc_mx and gt_pch_nx
functions in them that all get warned about... this will require messing with
the generator utilities to get them to stop generating code that warns, so this
will be more difficult than I thought; removing the "easyhack" keyword...