Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Sandra Loosemore]

On 08/10/2016 04:06 AM, Paolo Bonzini wrote:


===
--- gcc/doc/invoke.texi (revision 239276)
+++ gcc/doc/invoke.texi (working copy)
@@ -4914,6 +4914,12 @@
  construct, known from C++, was introduced with ISO C99 and is by default
  allowed in GCC@.  It is not supported by ISO C90.  @xref{Mixed Declarations}.

+@item -Wexpansion-to-defined
+@opindex Wexpansion-to-defined
+Warn whenever @samp{defined} is encountered in the expansion of a macro.
+(including the case where the macro is expanded by an @samp{#if} directive).


You've got too many periods in that sentence, and I'd get rid of the 
parentheses, too:


Warn whenever @samp{defined} is encountered in the expansion of a macro,
including the case where the macro is expanded by an @samp{#if} directive.

-Sandra the nit-picky

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Paolo Bonzini]

On 10/08/2016 17:58, Joseph Myers wrote:
> On Wed, 10 Aug 2016, Paolo Bonzini wrote:
> 
>> There are indeed many pedwarn(loc, 0, ...) occurrences in C++ (most, but
>> not all, are "foo only available with -std=bar" which in the C front-end
>> would use OPT_Wpedantic, OPT_W*compat be enabled by specific flags such
>> as -Wvariadic-macros).  In C I only see three:
> 
> I don't know why you think there are only three; there are loads.  The 
> first few in c-decl.c are:
> 
> pedwarn (csi->location, 0,
>  "%qD is static but used in inline function %qD "
>  "which is not static", csi->static_decl, csi->function);
> 
> pedwarn (csi->location, 0,
>  "%q+D is static but declared in inline function %qD "
>  "which is not static", csi->static_decl, csi->function);
> 
> pedwarn (input_location, 0,
>  "inline function %q+D declared but never defined", 
> p);
> 
>   pedwarned = pedwarn (input_location, 0,
>"conflicting types for %q+D", newdecl);
> 
>   pedwarned = pedwarn (input_location, 0,
>"conflicting types for %q+D", newdecl);
> 
>   pedwarn (input_location, 0,
>"unnamed struct/union that defines no instances");
> 

Hmm I was searching toplevel and c-family/.  I forgot that c/ now
exists, my knowledge is getting very dated...

Paolo

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Joseph Myers]

On Wed, 10 Aug 2016, Paolo Bonzini wrote:

> There are indeed many pedwarn(loc, 0, ...) occurrences in C++ (most, but
> not all, are "foo only available with -std=bar" which in the C front-end
> would use OPT_Wpedantic, OPT_W*compat be enabled by specific flags such
> as -Wvariadic-macros).  In C I only see three:

I don't know why you think there are only three; there are loads.  The 
first few in c-decl.c are:

pedwarn (csi->location, 0,
 "%qD is static but used in inline function %qD "
 "which is not static", csi->static_decl, csi->function);

pedwarn (csi->location, 0,
 "%q+D is static but declared in inline function %qD "
 "which is not static", csi->static_decl, csi->function);

pedwarn (input_location, 0,
 "inline function %q+D declared but never defined", p);

  pedwarned = pedwarn (input_location, 0,
   "conflicting types for %q+D", newdecl);

  pedwarned = pedwarn (input_location, 0,
   "conflicting types for %q+D", newdecl);

  pedwarn (input_location, 0,
   "unnamed struct/union that defines no instances");

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Paolo Bonzini]

On 10/08/2016 17:33, Joseph Myers wrote:
> On Wed, 10 Aug 2016, Paolo Bonzini wrote:
> 
>> - stuff that is not enabled by anything should use OPT_Wpedantic, and
> 
> No, lots of pedwarns are for usages that are (a) dubious enough we want to 
> diagnose them by default, and (b) required to be diagnosed by ISO C so 
> must become errors with -pedantic-errors.  (You could argue about whether 
> we should have -fpermissive for C like for C++ and make some of those into 
> errors by default.)

There are indeed many pedwarn(loc, 0, ...) occurrences in C++ (most, but
not all, are "foo only available with -std=bar" which in the C front-end
would use OPT_Wpedantic, OPT_W*compat be enabled by specific flags such
as -Wvariadic-macros).  In C I only see three:

- overflowing floating-point constants:

  if (!MODE_HAS_INFINITIES (TYPE_MODE (type)))
pedwarn (input_location, 0,
 "floating constant exceeds range of %qT", type);
  else
warning (OPT_Woverflow,
 "floating constant exceeds range of %qT", type);

- __FUNCTION__ outside function scope

  if (!ix && !current_function_decl)
pedwarn (loc, 0, "%qD is not defined outside of function scope", decl);

- macro redefinition

For the first two it would not feel wrong at all to use permerror.  The
last one indeed is a very good example of a pedwarn.

Paolo

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Joseph Myers]

On Wed, 10 Aug 2016, Paolo Bonzini wrote:

> - stuff that is not enabled by anything should use OPT_Wpedantic, and

No, lots of pedwarns are for usages that are (a) dubious enough we want to 
diagnose them by default, and (b) required to be diagnosed by ISO C so 
must become errors with -pedantic-errors.  (You could argue about whether 
we should have -fpermissive for C like for C++ and make some of those into 
errors by default.)

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Paolo Bonzini]

On 10/08/2016 17:24, Manuel López-Ibáñez wrote:
> Perhaps we need something like -Wextra-pedantic, for things that are
> undefined by ISO C but defined by GNU. Thus, they would not trigger
> pedwarns and no error with -pedantic-errors.

I think this is overengineering it a bit.  If they are annoying, put
them in -Wpedantic; if they aren't too bad, put them in Wextra.

There is also the case (which includes -Wshift-negative-value) of things
that are undefined by ISO C and trapped by ubsan, but defined by GNU C.

Actually are pedwarns even necessary nowadays?  In theory they fall in
two categories:

- stuff that is EnabledBy(Wpedantic) automatically gets bumped from
warning to error by -pedantic-errors or (I think) -Werror=pedantic;

- stuff that is not enabled by anything should use OPT_Wpedantic, and
then warning(OPT_Wpedantic) should have the same effect as
pedwarn(OPT_Wpedantic).

Are there other cases that I'm missing?

Paolo

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Manuel López-Ibáñez]

On 10 August 2016 at 15:44, Paolo Bonzini  wrote:
>
>
> On 10/08/2016 16:42, Manuel López-Ibáñez wrote:
>> > > My only fear is that people not using -Wpedantic nor -pedantic-errors
>> > > expect that GNU extensions work. This is a GNU extension that defines
>> > > something that is undefined according to ISO. Enabling the warning
>> > > with -Wextra is just annoying those people who may not care about
>> > > other compilers.
>> >
>> > I think this warning falls in the same category as
>> > -Wshift-negative-value.  (In fact I dislike -Wshift-negative-value a
>> > lot, and would put that one under -Wpedantic only).
>>
>> It is not the same category. One is compile-time UB and the other is
>> runtime UB. See:
>> https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01551.html
>> and https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01529.html
>
> Right---what I meant is it's the same kind of "annoying for people who
> expect that GNU extensions work" warning.

Oh, I agree. I'm just mentioning what the current definition/behavior
is (and documenting it here:
https://gcc.gnu.org/wiki/DiagnosticsGuidelines FWIW), not what I think
should be.

Perhaps we need something like -Wextra-pedantic, for things that are
undefined by ISO C but defined by GNU. Thus, they would not trigger
pedwarns and no error with -pedantic-errors.

Or we need to split -Wpedantic into -Wpedantic-pedwarns and
-Wpedantic-nopedwarns (with better names). This way -pedantic-errors
would be equivalent to -Werror=pedantic-pedwarns +
-Werror=pedwarns-not-controlled-by-pedantic.

i find -pedantic-errors too out of place with the rest of -W* options.

Manuel.

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Paolo Bonzini]

On 10/08/2016 16:42, Manuel López-Ibáñez wrote:
> > > My only fear is that people not using -Wpedantic nor -pedantic-errors
> > > expect that GNU extensions work. This is a GNU extension that defines
> > > something that is undefined according to ISO. Enabling the warning
> > > with -Wextra is just annoying those people who may not care about
> > > other compilers.
> >
> > I think this warning falls in the same category as
> > -Wshift-negative-value.  (In fact I dislike -Wshift-negative-value a
> > lot, and would put that one under -Wpedantic only).
>
> It is not the same category. One is compile-time UB and the other is
> runtime UB. See:
> https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01551.html
> and https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01529.html

Right---what I meant is it's the same kind of "annoying for people who
expect that GNU extensions work" warning.

Paolo

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Manuel López-Ibáñez]

On 10 August 2016 at 13:06, Paolo Bonzini  wrote:
>
>
> On 10/08/2016 13:31, Manuel López-Ibáñez wrote:
>> My only fear is that people not using -Wpedantic nor -pedantic-errors
>> expect that GNU extensions work. This is a GNU extension that defines
>> something that is undefined according to ISO. Enabling the warning
>> with -Wextra is just annoying those people who may not care about
>> other compilers.
>
> I think this warning falls in the same category as
> -Wshift-negative-value.  (In fact I dislike -Wshift-negative-value a
> lot, and would put that one under -Wpedantic only).

It is not the same category. One is compile-time UB and the other is
runtime UB. See:
https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01551.html
and https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01529.html

Cheers,

Manuel.

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Paolo Bonzini]

On 10/08/2016 13:31, Manuel López-Ibáñez wrote:
> My only fear is that people not using -Wpedantic nor -pedantic-errors
> expect that GNU extensions work. This is a GNU extension that defines
> something that is undefined according to ISO. Enabling the warning
> with -Wextra is just annoying those people who may not care about
> other compilers.

I think this warning falls in the same category as
-Wshift-negative-value.  (In fact I dislike -Wshift-negative-value a
lot, and would put that one under -Wpedantic only).

It is interesting that GCC has been relying for a long time on such
behavior in the HAVE_DESIGNATED_INITIALIZERS macro, despite GCC not
having any interest in compiling with -Wpedantic.  I think this
reinforces the choice of adding the warning to -Wextra.

People using -Wextra are used to having to remove some warnings
manually, and this one probably doesn't have many hits (1 in QEMU, which
is what motivated me to add it to GCC; and 2 in GCC not counting the
duplicate code between gcc/system.h and libcpp/system.h).

Paolo

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Joseph Myers]

On Wed, 10 Aug 2016, Manuel López-Ibáñez wrote:

> Thus, my opinion is that the current definition of -Wpedantic is too
> restrictive and it should contain the "in some cases where there is
> undefined behavior at compile-time". And thus, this should be a

Yes, it should.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH v2] cpp/c: Add -Wexpansion-to-defined

[Manuel López-Ibáñez]

On 10 August 2016 at 11:06, Paolo Bonzini  wrote:
> While I disagree with their inclusion of the warning to -Wall, I think
> it is a good addition overall.  First, it is a logical extension of the
> existing warning for breaking defined across a macro and its caller.
> Second, it is good to make these warnings for `defined' available with
> a command-line option other than -pedantic.  In fact this warning is
> not mandated by the standard and thus is a strange case of a non-pedwarn
> enabled by -pedantic.  As a side effect of using the command-line parsing
> machinery to attach the new warning to -pedantic, it would become an
> error for -pedantic-errors, which would be weird for a diagnostic that
> is not mandated by the standard.

Note that the definition of -pedantic-errors says: "in some cases
where there is undefined behavior at compile-time". Thus, this would
be allowed according to our current definitions. However, the
definition of -Wpedantic does not mention that, thus it could be a
pedwarn not controlled by -Wpedantic.

My only fear is that people not using -Wpedantic nor -pedantic-errors
expect that GNU extensions work. This is a GNU extension that defines
something that is undefined according to ISO. Enabling the warning
with -Wextra is just annoying those people who may not care about
other compilers.

Thus, my opinion is that the current definition of -Wpedantic is too
restrictive and it should contain the "in some cases where there is
undefined behavior at compile-time". And thus, this should be a
pedwarn enabled by -Wpedantic, not by -Wextra and an error with
-pedantic-errors. But you should wait for other opinions, specially
Joseph, before redoing it, even if you agree with me.

Cheers,

Manuel.