Changes in C++ FE regarding pedwarns to be errors are harmful

Changes in C++ FE regarding pedwarns to be errors are harmful

[Ismail Dönmez]

Hi all,

Looks like gcc 4.3 has some rather inconvenient changes in C++ FE, with the 
latest trunk. Lets see with an example :

[~]> cat test.cpp
#define foo bar
#define foo baz

[~]> g++ -c test.cpp
test.cpp:2:1: error: "foo" redefined
test.cpp:1:1: error: this is the location of the previous definition

I don't know the reasoning behind this change but this breaks many C++ 
programs unless -fpermissive is used. Why? Because everybody loves to install 
their own config.h (Python, libmp4v2 being nice examples) which just 
carelessly #define anything its asked for with ifndef ... endif .

Now flash back to real world: this breaks any C++ application that uses 
Python, libmp4v2, libjpeg and possibly many others. And I think this is a 
real bad behaviour change and I am not sure if its worth all the trouble.

So I am asking, would C++ FE maintainers would agree to revert this into a 
warning as in C FE now.

I welcome and value your comments, so please speak up.

Regards,
ismail

-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Joe Buck]

On Tue, Jan 08, 2008 at 11:28:22PM +0200, Ismail Dönmez wrote:
> Hi all,
> 
> Looks like gcc 4.3 has some rather inconvenient changes in C++ FE, with the 
> latest trunk. Lets see with an example :
> 
> [~]> cat test.cpp
> #define foo bar
> #define foo baz
> 
> [~]> g++ -c test.cpp
> test.cpp:2:1: error: "foo" redefined
> test.cpp:1:1: error: this is the location of the previous definition
> 
> I don't know the reasoning behind this change but this breaks many C++ 
> programs unless -fpermissive is used. Why? Because everybody loves to install 
> their own config.h (Python, libmp4v2 being nice examples) which just 
> carelessly #define anything its asked for with ifndef ... endif .
> 
> Now flash back to real world: this breaks any C++ application that uses 
> Python, libmp4v2, libjpeg and possibly many others. And I think this is a 
> real bad behaviour change and I am not sure if its worth all the trouble.

There's certainly an argument that this change is ill-advised.  However,
your statements in the last paragraph aren't true: most quality open
source projects have a "no warnings" rule (or at least try to eliminate
warnings), and most programmers know about #undef.  Since people have
already built whole distros with the gcc from the trunk, clearly they
are managing to build C++ applications that use Python, libmp4v2, libjpeg
etc.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Richard Guenther]

On Jan 8, 2008 10:34 PM, Joe Buck <Joe.Buck@synopsys.com> wrote:
> On Tue, Jan 08, 2008 at 11:28:22PM +0200, Ismail Dönmez wrote:
> > Hi all,
> >
> > Looks like gcc 4.3 has some rather inconvenient changes in C++ FE, with the
> > latest trunk. Lets see with an example :
> >
> > [~]> cat test.cpp
> > #define foo bar
> > #define foo baz
> >
> > [~]> g++ -c test.cpp
> > test.cpp:2:1: error: "foo" redefined
> > test.cpp:1:1: error: this is the location of the previous definition
> >
> > I don't know the reasoning behind this change but this breaks many C++
> > programs unless -fpermissive is used. Why? Because everybody loves to install
> > their own config.h (Python, libmp4v2 being nice examples) which just
> > carelessly #define anything its asked for with ifndef ... endif .
> >
> > Now flash back to real world: this breaks any C++ application that uses
> > Python, libmp4v2, libjpeg and possibly many others. And I think this is a
> > real bad behaviour change and I am not sure if its worth all the trouble.
>
> There's certainly an argument that this change is ill-advised.  However,
> your statements in the last paragraph aren't true: most quality open
> source projects have a "no warnings" rule (or at least try to eliminate
> warnings), and most programmers know about #undef.  Since people have
> already built whole distros with the gcc from the trunk, clearly they
> are managing to build C++ applications that use Python, libmp4v2, libjpeg
> etc.

Yep, in the worst case we stick a -fpermissive in.  See also PR33907 for
more obscure cases of the standard.

Richard.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Ismail Dönmez]

Tuesday 08 January 2008 23:34:13 tarihinde Joe Buck şunları yazmıştı:
> There's certainly an argument that this change is ill-advised.  However,
> your statements in the last paragraph aren't true: most quality open
> source projects have a "no warnings" rule (or at least try to eliminate
> warnings), and most programmers know about #undef.

Interesting assumption because knowing about #undef doesn't cut it because 
autoconf will happily #define without ifndef etc.

> Since people have  already built whole distros with the gcc from the trunk, 
> clearly theyare managing to build C++ applications that use 
> Python,libmp4v2, libjpeg etc. 

Yes true because they use -fpermissive which will let this error into a 
warning, which also suggests this change is not good.

Regards,
ismail

-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Andrew Pinski]

On 1/8/08, Ismail Dönmez <ismail@pardus.org.tr> wrote:
> Tuesday 08 January 2008 23:34:13 tarihinde Joe Buck şunları yazmıştı:
> > There's certainly an argument that this change is ill-advised. However,
> > your statements in the last paragraph aren't true: most quality open
> > source projects have a "no warnings" rule (or at least try to eliminate
> > warnings), and most programmers know about #undef.
>
> Interesting assumption because knowing about #undef doesn't cut it because
> autoconf will happily #define without ifndef etc.

The other thing is before this change, the C++ front-end was
inconstaint with the preprocessor and now it is constaint.  So adding
-pedantic-errors made those warnings into errors while doing
-pedantic-errors -fpermissive will still kept them as errors.
As shown by:

apinski@debian:~$ ~/x86-linux-4.0.2/bin/gcc t.cc -pedantic-errors -fpermissive
t.cc:2:1: error: "a" redefined
t.cc:1:1: error: this is the location of the previous definition
apinski@debian:~$ ~/x86-linux-4.0.2/bin/gcc t.cc
t.cc:2:1: warning: "a" redefined
t.cc:1:1: warning: this is the location of the previous definition
/usr/lib/crt1.o(.text+0x18): In function `_start':
../sysdeps/i386/elf/start.S:98: undefined reference to `main'
collect2: ld returned 1 exit status

Thanks,
Andrew Pinski

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Manuel López-Ibáñez]

On 08/01/2008, Ismail Dönmez <ismail@pardus.org.tr> wrote:
> Tuesday 08 January 2008 23:34:13 tarihinde Joe Buck şunları yazmıştı:
>
> > Since people have  already built whole distros with the gcc from the trunk,
> > clearly theyare managing to build C++ applications that use
> > Python,libmp4v2, libjpeg etc.
>
> Yes true because they use -fpermissive which will let this error into a
> warning, which also suggests this change is not good.
>

Hi Ismail,

I implemented the change as the fix to a bug that was reported by
fellow (and more senior) GCC developers. Let me try to explain
(although, I hoped that it will be fairly clear from
gcc.gnu.org/gcc-4.3/changes.html).

We have a series of diagnostics that are important or required by the
standard (or both). Those are called pedwarns. It is an unfortunate
name for two reasons: 1) Not all pedwarns are enabled by -pedantic,
some are enabled by default; and 2) pedwarns can be either warnings or
errors, and what they are depends on command-line options and the
default of each front-end.  In fact, pedwarnss are warnings by default
in the C front-end. But they are errors by default in the C++
front-end unless you use -fpermissive so they are downgraded to
warnings.

Before GCC 4.3, the C++ preprocessor was ignoring the default of the
C++ front-end. In fact, there was an inconsistence between the
settings of the C++ preprocessor and the settings of the C++
front-end. It worked for a few things (-pedantic-errors worked fine),
but not for others (the default settings and the effect of
-fpermissive). The inconsistency was evident in the code and if you
played with combinations of -pedantic-errors and -fpermissive.

I hope you see. It is not that we decided that some warnings should
actually be errors. It is that those warnings belong to a group of
diagnostics that were supposed to be errors by default. Of course, we
can discuss whether those warnings (or a particular one) should be
enabled by default, or whether they (or a particular one of them)
should belong to that group of diagnostics (pedwarns) or whether
pedwarns should be errors by default in C++ front-end.

I was hoping those discussions would arise but it didn't happen. It
seems people just used -fpermissive or fixed the warning.

For your particular example, you could open a regression bug against
4.3 that says:

* '"foo' redefined" is not mandated by the standard or it is not
serious enough, so it should not be a pedwarn just a normal warning;
or

* "'foo' redefined", although mandated by the standard, is a nuisance
and irrelevant, it should only be active when -pedantic is given and
not by default.

The fixes to those are one-liners once the correct decision is made. I
volunteer (in fact, I have volunteered repeatedly) to implement, test
and commit the changes needed. But I don't consider that I have the
expertise to make the correct decision.

But reverting my patch is just a quick hack that won't fix anything
for long, since in the future the preprocessor will probably use the
same diagnostic machinery that the front-ends, making impossible the
inconsistency described above and, thus, bringing up this whole issue
again.

I hope I have been clear in my explanation.

Cheers,

Manuel.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Ismail Dönmez]

Hi Manuel,

Wednesday 09 January 2008 00:28:54 tarihinde Manuel López-Ibáñez şunları 
yazmıştı:
> I implemented the change as the fix to a bug that was reported by
> fellow (and more senior) GCC developers. Let me try to explain
> (although, I hoped that it will be fairly clear from
> gcc.gnu.org/gcc-4.3/changes.html).

Yeah I know this change is documented at least.

> We have a series of diagnostics that are important or required by the
> standard (or both). Those are called pedwarns. It is an unfortunate
> name for two reasons: 1) Not all pedwarns are enabled by -pedantic,
> some are enabled by default; and 2) pedwarns can be either warnings or
> errors, and what they are depends on command-line options and the
> default of each front-end.  In fact, pedwarnss are warnings by default
> in the C front-end. But they are errors by default in the C++
> front-end unless you use -fpermissive so they are downgraded to
> warnings.
>
> Before GCC 4.3, the C++ preprocessor was ignoring the default of the
> C++ front-end. In fact, there was an inconsistence between the
> settings of the C++ preprocessor and the settings of the C++
> front-end. It worked for a few things (-pedantic-errors worked fine),
> but not for others (the default settings and the effect of
> -fpermissive). The inconsistency was evident in the code and if you
> played with combinations of -pedantic-errors and -fpermissive.

Oh that clears up my confusion. So the right fix would be downgrading this 
redefinition problem to be pedwarn instead. But I see no point in creating a 
bug report if its just gonna be closed as invalid, so I hope we can discuss 
if its feasible to downgrade this error to be a pedwarn.

What I see right now this distributions plug -fpermissive in and just go away. 
But I don't like workarounding gcc this way. I hope we can all agree that 
downrading this error to be a pedwarn is the correct way to go.

Regards,
ismail

-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Andrew Pinski]

On 1/8/08, Ismail Dönmez <ismail@pardus.org.tr> wrote:
> Oh that clears up my confusion. So the right fix would be downgrading this
> redefinition problem to be pedwarn instead. But I see no point in creating a
> bug report if its just gonna be closed as invalid, so I hope we can discuss
> if its feasible to downgrade this error to be a pedwarn.

It is already a pedwarn.  Just the C++ front-end enables pedwarn as
being errors by default and downgrades them as being warnings with
-fpermissive.  This is the whole point of -fpermissive.

Thanks,
Andrew Pinski

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Manuel López-Ibáñez]

On 08/01/2008, Ismail Dönmez <ismail@pardus.org.tr> wrote:
>
> Oh that clears up my confusion. So the right fix would be downgrading this
> redefinition problem to be pedwarn instead. But I see no point in creating a
> bug report if its just gonna be closed as invalid, so I hope we can discuss
> if its feasible to downgrade this error to be a pedwarn.
>
> What I see right now this distributions plug -fpermissive in and just go away.
> But I don't like workarounding gcc this way. I hope we can all agree that
> downrading this error to be a pedwarn is the correct way to go.
>

I am sorry you didn't understand my email and didn't read it
completely. I tried my best.

Cheers,

Manuel.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Ismail Dönmez]

Wednesday 09 January 2008 00:51:32 tarihinde şunları yazmıştınız:
> > Oh that clears up my confusion. So the right fix would be downgrading
> > this redefinition problem to be pedwarn instead. But I see no point in
> > creating a bug report if its just gonna be closed as invalid, so I hope
> > we can discuss if its feasible to downgrade this error to be a pedwarn.
> >
> > What I see right now this distributions plug -fpermissive in and just go
> > away. But I don't like workarounding gcc this way. I hope we can all
> > agree that downrading this error to be a pedwarn is the correct way to
> > go.
>
> I am sorry you didn't understand my email and didn't read it
> completely. I tried my best.

Sorry for misunderstanding, Andrew pointed out how I get it all wrong. Reading 
your mail again, you propose two possible fixes. Like you I am not in the 
position to see which would be the best solution.

So we need C++ FE maintainers to comment if they agree with one of those fixes 
at least. Though for me the first solution looks more plausible.

Thanks again,
ismail

-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Ismail Dönmez]

Hi again,

Wednesday 09 January 2008 00:28:54 tarihinde Manuel López-Ibáñez şunları 
yazmıştı:
> For your particular example, you could open a regression bug against
> 4.3 that says:
> * '"foo' redefined" is not mandated by the standard or it is not
> serious enough, so it should not be a pedwarn just a normal warning;
> or

Looks like this is actually mandated by standard :-( , thats what I am told on 
#gcc anyway :)

> * "'foo' redefined", although mandated by the standard, is a nuisance
> and irrelevant, it should only be active when -pedantic is given and
> not by default.

I am not sure if this is irrelevant for all cases, so the current behaviour 
looks correct. The right way would be fixing all those apps not installing 
their config.h and not pollute global namespace but thats a huge task given 
that many projects are affected.

So for now I guess we'll have to stick to -fpermissive. Thanks for your time 
and patience.

Regards,
ismail

-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Manuel López-Ibáñez]

On 09/01/2008, Ismail Dönmez <ismail@pardus.org.tr> wrote:
>
> Looks like this is actually mandated by standard :-( , thats what I am told on
> #gcc anyway :)
>

Not surprising since it is a pedwarn. It would be nice to point to the
relevant sections of the standard in the code as a comment, if you
know them. We do this for other pedwarns.

> I am not sure if this is irrelevant for all cases, so the current behaviour
> looks correct. The right way would be fixing all those apps not installing
> their config.h and not pollute global namespace but thats a huge task given
> that many projects are affected.
>
> So for now I guess we'll have to stick to -fpermissive. Thanks for your time
> and patience.
>

Of course there is a third option:

* Make pedwarns warnings by default unless -Werror or
--pedantic-errors are given (just like the C front-end).

I personally think that being pedantic when you have the possibility
of being permissive is not nice. In the current situation, it makes
completely sense to use -fpermissive always for the user flags (as
opposed to flags used during development) in a similar way as it makes
sense to never use -Werror for user flags. The next step is asking why
-fpermissive is not the default.

However, I was not around when this decision was taken, so there may
have been very good reasons for the current default. And perhaps those
reasons are still relevant nowadays.

Cheers,

Manuel.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Paolo Bonzini]

Andrew Pinski wrote:
> On 1/8/08, Ismail Dönmez <ismail@pardus.org.tr> wrote:
>> Oh that clears up my confusion. So the right fix would be downgrading this
>> redefinition problem to be pedwarn instead. But I see no point in creating a
>> bug report if its just gonna be closed as invalid, so I hope we can discuss
>> if its feasible to downgrade this error to be a pedwarn.
> 
> It is already a pedwarn.  Just the C++ front-end enables pedwarn as
> being errors by default and downgrades them as being warnings with
> -fpermissive.  This is the whole point of -fpermissive.

Not at all!!! -fpermissive can (in weird cases, agreed) change code 
generation.  I'm pretty sure you don't want to risk that only to silence 
an error.

I wonder if this requires a new diagnostic category, something like:

   if (pedantic)
     pedwarn ("...")
   else
     warning ("...")

Paolo

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Manuel López-Ibáñez]

On 09/01/2008, Paolo Bonzini <bonzini@gnu.org> wrote:
> Andrew Pinski wrote:
> > On 1/8/08, Ismail Dönmez <ismail@pardus.org.tr> wrote:
> >> Oh that clears up my confusion. So the right fix would be downgrading this
> >> redefinition problem to be pedwarn instead. But I see no point in creating a
> >> bug report if its just gonna be closed as invalid, so I hope we can discuss
> >> if its feasible to downgrade this error to be a pedwarn.
> >
> > It is already a pedwarn.  Just the C++ front-end enables pedwarn as
> > being errors by default and downgrades them as being warnings with
> > -fpermissive.  This is the whole point of -fpermissive.
>
> Not at all!!! -fpermissive can (in weird cases, agreed) change code
> generation.  I'm pretty sure you don't want to risk that only to silence
> an error.

What? That doesn't make any sense. And it is certainly not documented
in the manual. I will be very interested in an example, no matter how
weird or uncommon.

As far as I understand, if you get an error, no code is generated. If
you use -fpermissive and downgrade it to a warning, then code is
generated. Yes, no code is different from code but I won't call that
"changing code generation". If the code is wrong, then what is the
point of allowing it to be downgraded to a warning? If the code is not
conforming that is what the warning is telling you anyway and if you
ignore, you assume your risk.

So, I still think my analysis above is valid.

Cheers,

Manuel.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Paolo Bonzini]

>> Not at all!!! -fpermissive can (in weird cases, agreed) change code
>> generation.  I'm pretty sure you don't want to risk that only to silence
>> an error.
> 
> What? That doesn't make any sense. And it is certainly not documented
> in the manual. I will be very interested in an example, no matter how
> weird or uncommon.

Uhm, I might be confusing it with -ffriend-injection (which is in some 
sense more permissive than -fno-friend-injection).

Reading "in some case name lookup will not be standard conforming" in 
bkoz's document at http://people.redhat.com/~bkoz/porting_to_gcc43.html 
added to my confusion.

Paolo

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Ian Lance Taylor]

"Manuel López-Ibáñez" <lopezibanez@gmail.com> writes:

> Of course there is a third option:
> 
> * Make pedwarns warnings by default unless -Werror or
> --pedantic-errors are given (just like the C front-end).

This makes sense to me.  I have never understood why it is a good idea
for the C++ and C frontends to differ in this way.

Ian

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Benjamin Kosnik]

>> Of course there is a third option:
>> * Make pedwarns warnings by default unless -Werror or
>> --pedantic-errors are given (just like the C front-end).

>This makes sense to me.  I have never understood why it is a good idea
>for the C++ and C frontends to differ in this way.

Me too. The current error behavior just seems gratuitous. What was the
rationale for this change to error instead of warn? I am having
problems locating this discussion on gcc-patches.

-benjamin

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Andrew Pinski]

On 1/9/08, Benjamin Kosnik <bkoz@redhat.com> wrote:
> Me too. The current error behavior just seems gratuitous. What was the
> rationale for this change to error instead of warn? I am having
> problems locating this discussion on gcc-patches.

The recent preprocessor change or the older front-end change?

The older front-end change was done at
http://gcc.gnu.org/ml/gcc-patches/1998-12/msg00137.html .

The preprocessor change was done so the front-end was constaint with
the preprocessor and this was
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=24924.

Thanks,
Andrew Pinski

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Paolo Carlini]

Andrew Pinski wrote:
> constaint
*consistent*

Paolo.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Mark Mitchell]

Ian Lance Taylor wrote:
> "Manuel López-Ibáñez" <lopezibanez@gmail.com> writes:
> 
>> Of course there is a third option:
>>
>> * Make pedwarns warnings by default unless -Werror or
>> --pedantic-errors are given (just like the C front-end).
> 
> This makes sense to me.  I have never understood why it is a good idea
> for the C++ and C frontends to differ in this way.

I think Jason's input would be helpful.  I remember having a discussion
about this years ago (1998?), but I don't remember the complete
rationale.  I think the idea was that we wanted many of these things
(ugly old ARM-era C++ things) to be errors, but didn't want to make it
impossible to compile old code.  They're not "pedantic" in the sense
that you only care if you're trying extremely hard to be ISO-conformant;
 they're things no sane C++ programmer would do at this point, but we
want to support for legacy C++ code.

I don't see any a priori problem with changing to match the C front end.
 We could of course change some of the pedwarns into errors if we really
think they ought to be errors.  Or, some of them could be ordinary
warnings when not -pednatic, and pedwarns when -pedantic.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Jason Merrill]

Mark Mitchell wrote:
  > I think Jason's input would be helpful.  I remember having a discussion
> about this years ago (1998?), but I don't remember the complete
> rationale.  I think the idea was that we wanted many of these things
> (ugly old ARM-era C++ things) to be errors, but didn't want to make it
> impossible to compile old code.  They're not "pedantic" in the sense
> that you only care if you're trying extremely hard to be ISO-conformant;
> they're things no sane C++ programmer would do at this point, but we
> want to support for legacy C++ code.

Right. -fpermissive was a kludgey way to make these things errors by 
default but still allow them to compile.

> I don't see any a priori problem with changing to match the C front end.
>  We could of course change some of the pedwarns into errors if we really
> think they ought to be errors.  Or, some of them could be ordinary
> warnings when not -pedantic, and pedwarns when -pedantic.

Sounds like we want a separate category of diagnostic with the current 
C++ pedwarn semantics so that we can change pedwarns themselves back to 
a warning by default.

Jason

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Joe Buck]

Mark Mitchell wrote:
> >I don't see any a priori problem with changing to match the C front end.
> > We could of course change some of the pedwarns into errors if we really
> >think they ought to be errors.  Or, some of them could be ordinary
> >warnings when not -pedantic, and pedwarns when -pedantic.

On Fri, Jan 11, 2008 at 12:01:41PM -0500, Jason Merrill wrote:
> Sounds like we want a separate category of diagnostic with the current 
> C++ pedwarn semantics so that we can change pedwarns themselves back to 
> a warning by default.

Agreed.  Some have argued that the change makes sense because of
consistency arguments.  I'm not impressed with that; the compiler
is designed to be used, so the question is what most serves the users.

If distros wind up having to use -fpermissive extensively, where they
didn't before, we have a problem.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Mark Mitchell]

Joe Buck wrote:
> Mark Mitchell wrote:
>>> I don't see any a priori problem with changing to match the C front end.
>>> We could of course change some of the pedwarns into errors if we really
>>> think they ought to be errors.  Or, some of them could be ordinary
>>> warnings when not -pedantic, and pedwarns when -pedantic.

>> Sounds like we want a separate category of diagnostic with the current 
>> C++ pedwarn semantics so that we can change pedwarns themselves back to 
>> a warning by default.
> 
> Agreed.  Some have argued that the change makes sense because of
> consistency arguments.  I'm not impressed with that; the compiler
> is designed to be used, so the question is what most serves the users.

Exactly so.  I think that we have two kinds of pedwarns: those that are
pedantic in the sense we use for C (like, that there cannot be a naked
semicolon at the top-level of a file, or that "long long" is not in
C++98) and those that refer to semantically reasonable constructs that
we previously accepted, often because they were allowed by cfront or the
ARM.  With flag_permissive, we probably want the latter category to be
warnings at most; without flag_permissive, we want them to be errors.

This is a conceptually simple project, but it will take some work to
implement it, because someone will have to look at every pedwarn and
decide which category they are in.  Any volunteers?

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Jonathan Wakely]

On 11/01/2008, Mark Mitchell wrote:
>
> Exactly so.  I think that we have two kinds of pedwarns: those that are
> pedantic in the sense we use for C (like, that there cannot be a naked
> semicolon at the top-level of a file, or that "long long" is not in
> C++98) and those that refer to semantically reasonable constructs that
> we previously accepted, often because they were allowed by cfront or the
> ARM.  With flag_permissive, we probably want the latter category to be
> warnings at most; without flag_permissive, we want them to be errors.

I'll start the ball rolling with cp/call.c, cp/class.c and cp/cvt.c
I'll call the latter category isowarns for the purpose of this mail,
they are technically illegal constructs that are errors without
-fpermissive.  My suggestion of pedwarn/isowarn given in capitals on
each entry.

call.c:3258 build_conditional_expr  PEDWARN
"ISO C++ forbids omitting the middle term of a ?: expression."
GNU extension, currently allowed unless -pedantic.

call.c:3867 build_new_op ISOWARN
"no 'operator++(int)' declared for postfix '++', trying prefix operator instead"
This should not be accepted without -fpermissive

call.c:4352 convert_like_real ISOWARN
  pedwarn ("invalid conversion from %qT to %qT", TREE_TYPE (expr), totype);
  if (fn)
    pedwarn ("  initializing argument %P of %qD", argnum, fn);
Not sure about this one ... isowarn I think.

call.c:4953 build_over_call ISOWARN
"passing 'const X' as 'this' argument of 'void X::f()' discards qualifiers"

call.c:6463 joust PEDWARN
"ISO C++ says that these are ambiguous, even though the worst
conversion for the first is better than the worst conversion for the
second:"
GNU extension, currently entirely disallowed by -pedantic, otherwise pedwarn.
I think that's OK.

class.c:2483 finish_struct_anon ISOWARN
""%q+#D invalid; an anonymous struct can only have non-static data members"
"private member %q+#D in anonymous struct"
(and similarly for anonymous unions adn protected members)
Not sure how to trigger this, but I don't think is is intended as a
GNU extension so should require -fpermissive

class.c:3037 check_field_decls ISOWARN
"field 'int S::S' with same name as class"

class.c:5995 resolve_address_of_overloaded_function ISOWARN
"assuming pointer to member %qD (a pointer to member can only be
formed with %<&%E%>)"

class.c:6358 note_name_declared_in_class ISOWARN
"declaration of 'i'
changes meaning of 'i' from '<anonymous enum> i'"

cvt.c:382 warn_ref_binding ISOWARN
"initialization of non-const reference type %q#T from rvalue of type %qT"

cvt.c:452 convert_to_reference ISOWARN
"conversion from %qT to %qT discards qualifiers"

cvt.c:656  ocp_convert ISOWARN
"conversion from %q#T to %q#T"

cvt.c:902 convert_to_void ???
"statement cannot resolve address of overloaded function"
Shouldn't this be a hard error?  What benefit is there to allowing
this with -fpermissive?


Jon

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Manuel López-Ibáñez]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

On 12/01/2008, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 11/01/2008, Mark Mitchell wrote:
> >
> > Exactly so.  I think that we have two kinds of pedwarns: those that are
> > pedantic in the sense we use for C (like, that there cannot be a naked
> > semicolon at the top-level of a file, or that "long long" is not in
> > C++98) and those that refer to semantically reasonable constructs that
> > we previously accepted, often because they were allowed by cfront or the
> > ARM.  With flag_permissive, we probably want the latter category to be
> > warnings at most; without flag_permissive, we want them to be errors.
>
> I'll start the ball rolling with cp/call.c, cp/class.c and cp/cvt.c
> I'll call the latter category isowarns for the purpose of this mail,
> they are technically illegal constructs that are errors without
> -fpermissive.  My suggestion of pedwarn/isowarn given in capitals on
> each entry.
>

Here is an initial patch implementing some of your proposals. I used
pederror as the name of the function. That is, it is an error unless
fpermissive is given.

Let me know your opinion. (Not bootstrapped, not tested, not complete patch).

Cheers,

Manuel.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pederror.diff --]
[-- Type: text/x-diff; name=pederror.diff, Size: 11649 bytes --]

Index: gcc/diagnostic.c
===================================================================
--- gcc/diagnostic.c	(revision 131405)
+++ gcc/diagnostic.c	(working copy)
@@ -538,15 +538,36 @@ pedwarn (const char *gmsgid, ...)
   diagnostic_info diagnostic;
   va_list ap;
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, input_location,
+		       pedantic_warning_kind ());
+  report_diagnostic (&diagnostic);
+  va_end (ap);
+}
+
+/* A "pedantic" error: issues an error unless -fpermissive was
+   given on the command line, in which case it issues a warning.  Use
+   this for things that really should be errors but we want to support legacy code.
+
+   Note that these diagnostics are issued independent of the setting
+   of the -pedantic command-line switch.  To get an error enabled
+   only with that switch, write "if (pedantic) pederror (...);"  */
+void
+pederror (const char *gmsgid, ...)
+{
+  diagnostic_info diagnostic;
+  va_list ap;
+
+  va_start (ap, gmsgid);
+  diagnostic_set_info (&diagnostic, gmsgid, &ap, input_location,
 		       pedantic_error_kind ());
   report_diagnostic (&diagnostic);
   va_end (ap);
 }
 
+
 /* A hard error: the code is definitely ill-formed, and an object file
    will not be produced.  */
 void
 error (const char *gmsgid, ...)
 {
Index: gcc/diagnostic.h
===================================================================
--- gcc/diagnostic.h	(revision 131405)
+++ gcc/diagnostic.h	(working copy)
@@ -48,11 +48,12 @@ typedef struct diagnostic_info
   diagnostic_t kind;
   /* Which OPT_* directly controls this diagnostic.  */
   int option_index;
 } diagnostic_info;
 
-#define pedantic_error_kind() (flag_pedantic_errors ? DK_ERROR : DK_WARNING)
+#define pedantic_warning_kind() (flag_pedantic_errors ? DK_ERROR : DK_WARNING)
+#define pedantic_error_kind() (flag_permissive ? DK_WARNING : DK_ERROR)
 
 
 /*  Forward declarations.  */
 typedef struct diagnostic_context diagnostic_context;
 typedef void (*diagnostic_starter_fn) (diagnostic_context *,
Index: gcc/toplev.h
===================================================================
--- gcc/toplev.h	(revision 131405)
+++ gcc/toplev.h	(working copy)
@@ -60,10 +60,11 @@ extern void warning0 (const char *, ...)
 extern void warning (int, const char *, ...) ATTRIBUTE_GCC_DIAG(2,3);
 extern void error (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2);
 extern void fatal_error (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2)
      ATTRIBUTE_NORETURN;
 extern void pedwarn (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2);
+extern void pederror (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2);
 extern void sorry (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2);
 extern void inform (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2);
 extern void verbatim (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2);
 
 extern void rest_of_decl_compilation (tree, int, int);
Index: gcc/cp/decl.c
===================================================================
--- gcc/cp/decl.c	(revision 131405)
+++ gcc/cp/decl.c	(working copy)
@@ -6981,16 +6981,11 @@ compute_array_index_type (tree name, tre
   /* Normally, the array-bound will be a constant.  */
   if (TREE_CODE (size) == INTEGER_CST)
     {
       /* Check to see if the array bound overflowed.  Make that an
 	 error, no matter how generous we're being.  */
-      int old_flag_pedantic_errors = flag_pedantic_errors;
-      int old_pedantic = pedantic;
-      pedantic = flag_pedantic_errors = 1;
-      constant_expression_warning (size);
-      pedantic = old_pedantic;
-      flag_pedantic_errors = old_flag_pedantic_errors;
+      constant_expression_error (size);
 
       /* An array must have a positive number of elements.  */
       if (INT_CST_LT (size, integer_zero_node))
 	{
 	  if (name)
Index: gcc/cp/call.c
===================================================================
--- gcc/cp/call.c	(revision 131405)
+++ gcc/cp/call.c	(working copy)
@@ -3862,11 +3862,11 @@ build_new_op (enum tree_code code, int f
 	case POSTINCREMENT_EXPR:
 	case POSTDECREMENT_EXPR:
 	  /* Look for an `operator++ (int)'.  If they didn't have
 	     one, then we fall back to the old way of doing things.  */
 	  if (flags & LOOKUP_COMPLAIN)
-	    pedwarn ("no %<%D(int)%> declared for postfix %qs, "
+	    pederror ("no %<%D(int)%> declared for postfix %qs, "
 		     "trying prefix operator instead",
 		     fnname,
 		     operator_name_info[code].name);
 	  if (code == POSTINCREMENT_EXPR)
 	    code = PREINCREMENT_EXPR;
@@ -4347,13 +4347,13 @@ convert_like_real (conversion *convs, tr
 				      /*issue_conversion_warnings=*/false,
 				      /*c_cast_p=*/false);
 	  else if (t->kind == ck_identity)
 	    break;
 	}
-      pedwarn ("invalid conversion from %qT to %qT", TREE_TYPE (expr), totype);
+      pederror ("invalid conversion from %qT to %qT", TREE_TYPE (expr), totype);
       if (fn)
-	pedwarn ("  initializing argument %P of %qD", argnum, fn);
+	pederror ("  initializing argument %P of %qD", argnum, fn);
       return cp_convert (totype, expr);
     }
 
   if (issue_conversion_warnings)
     conversion_null_warnings (totype, expr, fn, argnum);
@@ -4948,11 +4948,11 @@ build_over_call (struct z_candidate *can
       tree argtype = TREE_TYPE (TREE_VALUE (arg));
       tree converted_arg;
       tree base_binfo;
 
       if (convs[i]->bad_p)
-	pedwarn ("passing %qT as %<this%> argument of %q#D discards qualifiers",
+	pederror ("passing %qT as %<this%> argument of %q#D discards qualifiers",
 		 TREE_TYPE (argtype), fn);
 
       /* [class.mfct.nonstatic]: If a nonstatic member function of a class
 	 X is called for an object that is not of type X, or of a type
 	 derived from X, the behavior is undefined.
Index: gcc/cp/error.c
===================================================================
--- gcc/cp/error.c	(revision 131405)
+++ gcc/cp/error.c	(working copy)
@@ -2653,11 +2653,11 @@ cp_cpp_error (cpp_reader *pfile ATTRIBUT
     case CPP_DL_WARNING:
     case CPP_DL_WARNING_SYSHDR:
       dlevel = DK_WARNING;
       break;
     case CPP_DL_PEDWARN:
-      dlevel = pedantic_error_kind ();
+      dlevel = pedantic_warning_kind ();
       break;
     case CPP_DL_ERROR:
       dlevel = DK_ERROR;
       break;
     case CPP_DL_ICE:
Index: gcc/cp/lex.c
===================================================================
--- gcc/cp/lex.c	(revision 131405)
+++ gcc/cp/lex.c	(working copy)
@@ -658,20 +658,20 @@ unqualified_fn_lookup_error (tree name)
 	 is going wrong.
 
 	 Note that we have the exact wording of the following message in
 	 the manual (trouble.texi, node "Name lookup"), so they need to
 	 be kept in synch.  */
-      pedwarn ("there are no arguments to %qD that depend on a template "
+      pederror ("there are no arguments to %qD that depend on a template "
 	       "parameter, so a declaration of %qD must be available",
 	       name, name);
 
       if (!flag_permissive)
 	{
 	  static bool hint;
 	  if (!hint)
 	    {
-	      error ("(if you use %<-fpermissive%>, G++ will accept your "
+	      inform ("(if you use %<-fpermissive%>, G++ will accept your "
 		     "code, but allowing the use of an undeclared name is "
 		     "deprecated)");
 	      hint = true;
 	    }
 	}
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 131405)
+++ gcc/cp/parser.c	(working copy)
@@ -9724,11 +9724,11 @@ cp_parser_template_id (cp_parser *parser
 	  pop_deferring_access_checks ();
 	  return error_mark_node;
 	}
       /* Otherwise, emit an error about the invalid digraph, but continue
 	 parsing because we got our argument list.  */
-      pedwarn ("%<<::%> cannot begin a template-argument list");
+      pederror ("%<<::%> cannot begin a template-argument list");
       inform ("%<<:%> is an alternate spelling for %<[%>. Insert whitespace "
 	      "between %<<%> and %<::%>");
       if (!flag_permissive)
 	{
 	  static bool hint;
Index: gcc/c-errors.c
===================================================================
--- gcc/c-errors.c	(revision 131405)
+++ gcc/c-errors.c	(working copy)
@@ -36,11 +36,11 @@ pedwarn_c99 (const char *gmsgid, ...)
   diagnostic_info diagnostic;
   va_list ap;
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, input_location,
-		       flag_isoc99 ? pedantic_error_kind () : DK_WARNING);
+		       flag_isoc99 ? pedantic_warning_kind () : DK_WARNING);
   report_diagnostic (&diagnostic);
   va_end (ap);
 }
 
 /* Issue an ISO C90 pedantic warning MSGID.  This function is supposed to
@@ -54,9 +54,9 @@ pedwarn_c90 (const char *gmsgid, ...)
   diagnostic_info diagnostic;
   va_list ap;
 
   va_start (ap, gmsgid);
   diagnostic_set_info (&diagnostic, gmsgid, &ap, input_location,
-		       flag_isoc99 ? DK_WARNING : pedantic_error_kind ());
+		       flag_isoc99 ? DK_WARNING : pedantic_warning_kind ());
   report_diagnostic (&diagnostic);
   va_end (ap);
 }
Index: gcc/c-opts.c
===================================================================
--- gcc/c-opts.c	(revision 131405)
+++ gcc/c-opts.c	(working copy)
@@ -1095,15 +1095,10 @@ c_common_post_options (const char **pfil
     warn_overlength_strings = 0;
 
   /* Adjust various flags for C++ based on command-line settings.  */
   if (c_dialect_cxx ())
     {
-      if (!flag_permissive)
-	{
-	  flag_pedantic_errors = 1;
-	  cpp_opts->pedantic_errors = 1;
-	}
       if (!flag_no_inline)
 	{
 	  flag_inline_trees = 1;
 	  flag_no_inline = 1;
 	}
Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c	(revision 131405)
+++ gcc/c-common.c	(working copy)
@@ -929,18 +929,28 @@ fix_string_type (tree value)
    constant expression to overflow.  */
 
 void
 constant_expression_warning (tree value)
 {
-  if ((TREE_CODE (value) == INTEGER_CST || TREE_CODE (value) == REAL_CST
-       || TREE_CODE (value) == FIXED_CST
-       || TREE_CODE (value) == VECTOR_CST
-       || TREE_CODE (value) == COMPLEX_CST)
-      && TREE_OVERFLOW (value)
-      && warn_overflow
-      && pedantic)
-    pedwarn ("overflow in constant expression");
+  if (warn_overflow && pedantic && TREE_OVERFLOW (value) 
+      && (TREE_CODE (value) == INTEGER_CST || TREE_CODE (value) == REAL_CST
+	  || TREE_CODE (value) == FIXED_CST
+	  || TREE_CODE (value) == VECTOR_CST
+	  || TREE_CODE (value) == COMPLEX_CST))
+      pedwarn ("overflow in constant expression");
+}
+
+/* The same as above but print an unconditional error.  */
+void
+constant_expression_error (tree value)
+{
+  if (TREE_OVERFLOW (value) 
+      && (TREE_CODE (value) == INTEGER_CST || TREE_CODE (value) == REAL_CST
+	  || TREE_CODE (value) == FIXED_CST
+	  || TREE_CODE (value) == VECTOR_CST
+	  || TREE_CODE (value) == COMPLEX_CST))
+      error ("overflow in constant expression");
 }
 
 /* Print a warning if an expression had overflow in folding and its
    operands hadn't.
 
Index: gcc/c-common.h
===================================================================
--- gcc/c-common.h	(revision 131405)
+++ gcc/c-common.h	(working copy)
@@ -686,10 +686,11 @@ extern tree c_alignof_expr (tree);
    NOP_EXPR is used as a special case (see truthvalue_conversion).  */
 extern void binary_op_error (enum tree_code, tree, tree);
 extern tree fix_string_type (tree);
 struct varray_head_tag;
 extern void constant_expression_warning (tree);
+extern void constant_expression_error (tree);
 extern bool strict_aliasing_warning (tree, tree, tree);
 extern void empty_if_body_warning (tree, tree);
 extern void warnings_for_convert_and_check (tree, tree, tree);
 extern tree convert_and_check (tree, tree);
 extern void overflow_warning (tree);

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Jonathan Wakely]

On 12/01/2008, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
>
> Here is an initial patch implementing some of your proposals. I used
> pederror as the name of the function. That is, it is an error unless
> fpermissive is given.

Ah, very fast! :-)
I was just starting somethign similar, I provisionally used the name
permerror, with this comment:

/* A conformance error: issues an error unless -fpermissive was
   given on the command line, in which case it issues a warning.  Use
   this for diagnostics required by the relevant language standard,
   but which need to be downgradable for non-conformant code.  */
void
permerror (const char *gmsgid, ...)

I don't think the name should involve "ped" or pedantic, since it's
not related to -pedantic, the semantics are those described by the
-fpermissive docs:

       -fpermissive
           Downgrade some diagnostics about nonconformant code from errors to
           warnings.  Thus, using -fpermissive will allow some nonconforming
           code to compile.

These errors should be independent of -pedantic* unless the if
(pedantic) check is also present in the code, but that's a special
case.  Have I understood the intention correctly?

I think the name matters, otherwise you will have to write more long
mails explaining why the semantics don't match the name :-)
(thanks for those mails btw, they helped me a lot)
I don't really like the name permerror either though, relaxable_error
is accurate but not great either.

Thanks for doing this - I agree with your other decisions about what
should be a pedward or a pederror/whatever in e.g. lex.c and parser.c

Jon

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Manuel López-Ibáñez]

On 12/01/2008, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 12/01/2008, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> >
> > Here is an initial patch implementing some of your proposals. I used
> > pederror as the name of the function. That is, it is an error unless
> > fpermissive is given.
>
> These errors should be independent of -pedantic* unless the if
> (pedantic) check is also present in the code, but that's a special
> case.  Have I understood the intention correctly?

Actually, I didn't understand that we wanted to separate fpermissive
and pedantic-errors completely. (Notice that the internal
declaration/definition of fpermissive actually mentions pedantic.) But
I agree with you: they should be unrelated.

> I don't really like the name permerror either though, relaxable_error
> is accurate but not great either.

I actually like permerror better than the alternative
permissive_error. I think those two are the only ones that make sense.

> Thanks for doing this - I agree with your other decisions about what
> should be a pedward or a pederror/whatever in e.g. lex.c and parser.c

Your information above was very helpful. But I am still not sure about
the rest of pedwarns.

Cheers,

Manuel.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Gabriel Dos Reis]

Ismail Dönmez <ismail@pardus.org.tr> writes:

| Hi again,
| 
| Wednesday 09 January 2008 00:28:54 tarihinde Manuel López-Ibáñez şunları 
| yazmıştı:
| > For your particular example, you could open a regression bug against
| > 4.3 that says:
| > * '"foo' redefined" is not mandated by the standard or it is not
| > serious enough, so it should not be a pedwarn just a normal warning;
| > or
| 
| Looks like this is actually mandated by standard :-( , thats what I am told on 
| #gcc anyway :)

    #define foo bar
    #define foo baz

is asking for trouble -- one should look for fixing the source of that
inconsistency. 

-- Gaby

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Ismail Dönmez]

Sunday 13 January 2008 17:41:03 tarihinde Gabriel Dos Reis şunları yazmıştı:
> Ismail Dönmez <ismail@pardus.org.tr> writes:
> | Hi again,
> |
> | Wednesday 09 January 2008 00:28:54 tarihinde Manuel López-Ibáñez şunları
> |
> | yazmıştı:
> | > For your particular example, you could open a regression bug against
> | > 4.3 that says:
> | > * '"foo' redefined" is not mandated by the standard or it is not
> | > serious enough, so it should not be a pedwarn just a normal warning;
> | > or
> |
> | Looks like this is actually mandated by standard :-( , thats what I am
> | told on #gcc anyway :)
>
>     #define foo bar
>     #define foo baz
>
> is asking for trouble -- one should look for fixing the source of that
> inconsistency.

That was just an example, real life testcase shows that problem stems from 
autoconf and its config.h. Projects end up defining things like HAVE_STDLIB_H 
twice which is not harmful at all but now causes an error if g++ is used.

Regards,
ismail

-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Andreas Schwab]

Ismail Dönmez <ismail@pardus.org.tr> writes:

> That was just an example, real life testcase shows that problem stems from 
> autoconf and its config.h. Projects end up defining things like HAVE_STDLIB_H 
> twice which is not harmful at all but now causes an error if g++ is used.

Redefinitions with the same replacement list are not an error.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Ismail Dönmez]

Sunday 13 January 2008 18:03:20 tarihinde Andreas Schwab şunları yazmıştı:
> Ismail Dönmez <ismail@pardus.org.tr> writes:
> > That was just an example, real life testcase shows that problem stems
> > from autoconf and its config.h. Projects end up defining things like
> > HAVE_STDLIB_H twice which is not harmful at all but now causes an error
> > if g++ is used.
>
> Redefinitions with the same replacement list are not an error.

Ok but that still doesn't cover the general use of

#define PACKAGE_NAME foobar

etc.

Regards,
ismail

-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Richard Guenther]

On Jan 13, 2008 5:10 PM, Ismail Dönmez <ismail@pardus.org.tr> wrote:
> Sunday 13 January 2008 18:03:20 tarihinde Andreas Schwab şunları yazmıştı:
> > Ismail Dönmez <ismail@pardus.org.tr> writes:
> > > That was just an example, real life testcase shows that problem stems
> > > from autoconf and its config.h. Projects end up defining things like
> > > HAVE_STDLIB_H twice which is not harmful at all but now causes an error
> > > if g++ is used.
> >
> > Redefinitions with the same replacement list are not an error.
>
> Ok but that still doesn't cover the general use of
>
> #define PACKAGE_NAME foobar
>
> etc.

But that's just bugs that need to be fixed.

Richard.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Ismail Dönmez]

Sunday 13 January 2008 18:10:00 tarihinde Richard Guenther şunları yazmıştı:
> On Jan 13, 2008 5:10 PM, Ismail Dönmez <ismail@pardus.org.tr> wrote:
> > Sunday 13 January 2008 18:03:20 tarihinde Andreas Schwab şunları yazmıştı:
> > > Ismail Dönmez <ismail@pardus.org.tr> writes:
> > > > That was just an example, real life testcase shows that problem stems
> > > > from autoconf and its config.h. Projects end up defining things like
> > > > HAVE_STDLIB_H twice which is not harmful at all but now causes an
> > > > error if g++ is used.
> > >
> > > Redefinitions with the same replacement list are not an error.
> >
> > Ok but that still doesn't cover the general use of
> >
> > #define PACKAGE_NAME foobar
> >
> > etc.
>
> But that's just bugs that need to be fixed.

I think we already agreed that this is just too many packages to fix and that 
this warning should only be an error with -pedantic. People already started 
adding #undef hacks to workaround this, others are building 
with -fpermissive.

So we should should just let Manu finish up his patch and get a review as C++ 
FE maintainers agreed as well.

Regards,
ismail


-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Gabriel Dos Reis]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1231 bytes --]

On Sun, 13 Jan 2008, Ismail Dönmez wrote:

| Sunday 13 January 2008 17:41:03 tarihinde Gabriel Dos Reis sunlar? yazm?st?:
| > Ismail Dönmez <ismail@pardus.org.tr> writes:
| > | Hi again,
| > |
| > | Wednesday 09 January 2008 00:28:54 tarihinde Manuel López-Ibáñez sunlar?
| > |
| > | yazm?st?:
| > | > For your particular example, you could open a regression bug against
| > | > 4.3 that says:
| > | > * '"foo' redefined" is not mandated by the standard or it is not
| > | > serious enough, so it should not be a pedwarn just a normal warning;
| > | > or
| > |
| > | Looks like this is actually mandated by standard :-( , thats what I am
| > | told on #gcc anyway :)
| >
| >     #define foo bar
| >     #define foo baz
| >
| > is asking for trouble -- one should look for fixing the source of that
| > inconsistency.
| 
| That was just an example,

I understood that.

| real life testcase shows that problem stems from 
| autoconf and its config.h. Projects end up defining things like HAVE_STDLIB_H 
| twice which is not harmful at all but now causes an error if g++ is used.

The problem is that any semantics of the above -- that is not an error
or silent ignorance -- is order-dependent, which is worse to debug.

-- Gaby

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Ismail Dönmez]

Sunday 13 January 2008 18:40:25 tarihinde Gabriel Dos Reis şunları yazmıştı:
> | real life testcase shows that problem stems from
> | autoconf and its config.h. Projects end up defining things like
> | HAVE_STDLIB_H twice which is not harmful at all but now causes an error
> | if g++ is used.
>
> The problem is that any semantics of the above -- that is not an error
> or silent ignorance -- is order-dependent, which is worse to debug.

True but lots of real world applications are falling apart, is it worth to 
break them all anyway? Also with -pedantic you will still get an error. I 
think its a good compromise.

Regards,
ismail

-- 
Never learn by your mistakes, if you do you may never dare to try again.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Gabriel Dos Reis]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 678 bytes --]

On Sun, 13 Jan 2008, Ismail Dönmez wrote:

| Sunday 13 January 2008 18:40:25 tarihinde Gabriel Dos Reis sunlar? yazm?st?:
| > | real life testcase shows that problem stems from
| > | autoconf and its config.h. Projects end up defining things like
| > | HAVE_STDLIB_H twice which is not harmful at all but now causes an error
| > | if g++ is used.
| >
| > The problem is that any semantics of the above -- that is not an error
| > or silent ignorance -- is order-dependent, which is worse to debug.
| 
| True but lots of real world applications are falling apart,

I believe it has been reported the numbre of those (reported in an
earlier message) is an overstatement.

-- Gaby

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Jonathan Wakely]

On 13/01/2008, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> On 12/01/2008, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> > On 12/01/2008, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> > >
> > > Here is an initial patch implementing some of your proposals. I used
> > > pederror as the name of the function. That is, it is an error unless
> > > fpermissive is given.
> >
> > These errors should be independent of -pedantic* unless the if
> > (pedantic) check is also present in the code, but that's a special
> > case.  Have I understood the intention correctly?
>
> Actually, I didn't understand that we wanted to separate fpermissive
> and pedantic-errors completely. (Notice that the internal
> declaration/definition of fpermissive actually mentions pedantic.) But
> I agree with you: they should be unrelated.

I hadn't noticed that in the flag_permissive declaration, thanks for
pointing it out.  I think that comment should be changed to refer to
permerrors instead.  (I'm not sure about the bit about being ignored
if -pedantic is given ... is that true today?

To summarise, the proposal is

Introduce a new class of diagnostic, permerror, which is an error by
default, but -fpermissive makes it a warning.
Change most C++ FE pedwarns to permerrors
Stop using pedantic-errors by default for the C++ FE and CPP

Since most pedwarns are now permerrors, we still get errors by
default, or warnings with -fpermissive. However, now CPP doesn't have
to use pedantic-errors to align with the C++ FE, so the class of CPP
diagnostics that are causing problems will remain as warnings, as per
GCC 4.2
If you want errors from the preprocessor you can use
-fpedantic-errors, consistent with C.

I think this results in consistent behaviour for the front-end and
preprocessor for both C and C++ (which is the behaviour documented in
the manual.)  Most diagnostics will behave equivalently but there are
a few special cases where the C pedwarn semantics are preferable, so
shouldn't be switched to permerrors (most of these cases are only
enabled by -pedantic and give no diagnostic without)

> Your information above was very helpful. But I am still not sure about
> the rest of pedwarns.

for decl.c I'm not so sure which category some of them should be in.
I'll list the ones I think should stay as pedwarns, everything else
should be a permerror (IMHO)

line 3811 check_tag_decl
      if (TREE_CODE (declared_type) != UNION_TYPE && pedantic
	  && !in_system_header)
	pedwarn ("ISO C++ prohibits anonymous structs");
guarded by pedantic, looks like it's allowed as an extension, so leave
as a pedwarn.
This won't change the default behaviour but now when -pedantic is
given this will only be a warning

line 6922 check_static_variable_definition
  else if (pedantic && !INTEGRAL_TYPE_P (type))
    pedwarn ("ISO C++ forbids initialization of member constant "
	     "%qD of non-integral type %qT", decl, type);
Extension, guarded by pedantic, leave it as pedwarn

line 7007 compute_array_index_type
      /* As an extension we allow zero-sized arrays.  We always allow
	 them in system headers because glibc uses them.  */
      else if (integer_zerop (size) && pedantic && !in_system_header)
	{
	  if (name)
	    pedwarn ("ISO C++ forbids zero-size array %qD", name);
	  else
	    pedwarn ("ISO C++ forbids zero-size array");
guarded by pedantic, leave as pedwarn

line 7025 compute_array_index_type
  else if (pedantic && warn_vla != 0)
    {
      if (name)
	pedwarn ("ISO C++ forbids variable length array %qD", name);
      else
	pedwarn ("ISO C++ forbids variable length array");
    }
GNU extension, guarded by pedantic, leave as pedwarn

7655  grokdeclarator
  else if (pedantic || ! is_main)
    pedwarn ("ISO C++ forbids declaration of %qs with no type", name);
  else
    warning (OPT_Wreturn_type,
           "ISO C++ forbids declaration of %qs with no type", name);

interesting one, the comment above says:

      /* We handle `main' specially here, because 'main () { }' is so
	 common.  With no options, it is allowed.  With -Wreturn-type,
	 it is a warning.  It is only an error with -pedantic-errors.  */

That's true, but currently pedantic-errors is on by default.
I think this should become:

  else if (! is_main)
    permerror ("ISO C++ forbids declaration of %qs with no type", name);
  else if (pedantic)
    pedwarn ("ISO C++ forbids declaration of %qs with no type", name);
  else
    warning (OPT_Wreturn_type,
         "ISO C++ forbids declaration of %qs with no type", name);

That would make it an error anywhere except main, but in main it's a
warning with -pedantic or -Wreturn-type.  -pedantic-errors or -Werror
make it an error on main too.

7703 grokdeclarator
    pedwarn ("long, short, signed or unsigned used invalidly for %qs",
       name);
guarded by pedantic, leave as pedwarn

7808 grokdeclarator
      /* This was an error in C++98 (cv-qualifiers cannot be added to
	 a function type), but DR 295 makes the code well-formed by
	 dropping the extra qualifiers. */
      if (pedantic)
	{
	  tree bad_type = build_qualified_type (type, type_quals);
	  pedwarn ("ignoring %qV qualifiers added to function type %qT",
		   bad_type, type);
	}

This is currently allowed by default, but an error with -pedantic,
even though it's well-formed!  Stay as pedwarn so it's only a warning
with -pedantic

9053 grokdeclarator
guarded by pedantic, leave both as pedwarns

9146 grokdeclarator
guarded by pedantic, leave as pedwarn (but change the one above to permerror)

10013 grok_op_properties
guarded by pedantic, leave as pedwarn


I think all others should change to permerrors.


Jon

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Manuel López-Ibáñez]

On 13/01/2008, Ismail Dönmez <ismail@pardus.org.tr> wrote:
>
> So we should should just let Manu finish up his patch and get a review as C++
> FE maintainers agreed as well.
>

Patch sent for approval: http://gcc.gnu.org/ml/gcc-patches/2008-01/msg00583.html
Extra testing is welcome in case I missed something.

In particular, any reference to flag_pedantic_errors after a permerror
is a potential bug.
Also, suggestions of pedwarns that should actually be permerrors would
be helpful.

Cheers,

Manuel.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Jonathan Wakely]

On 13/01/2008, Jonathan Wakely wrote:
>
> I think all others should change to permerrors.

I only meant all others in cp/decl.c of course - here are the remaining files.
Again I've only listed the ones that should remain as pedwarns and
other special cases - most change to permerrors.

In cp/error.c we have

/* Warn about the use of variadic templates when appropriate.  */
void
maybe_warn_variadic_templates (void)
{
  if ((cxx_dialect == cxx98) && !in_system_header)
    /* We really want to suppress this warning in system headers,
       because libstdc++ uses variadic templates even when we aren't
       in C++0x mode. */
    pedwarn ("ISO C++ does not include variadic templates");
}

Another interesting one.  Variadic templates work in C++98 mode and
are used internally, but currently you have to say -fpermissive to use
them in user code. That flag is *usually* needed for
backward-compatibility, not enabling *future* extensions.
If this becomes a permerror that behaviour will be preserved, if it
stays as a pedwarn then users will only get a warning for
variadic-templates in C++98.
So is support for variadic templates in -std=gnu98 mode an official
extension, or a detail not-for-public-consumption?

the pedwarns in except.c, friend.c and init.c should be permerrors

at name-lookup.c:728 we have:
	      if (pedantic && ! DECL_IN_SYSTEM_HEADER (x))
		pedwarn ("redeclaration of %<wchar_t%> as %qT",
			 TREE_TYPE (x));
guarded by pedantic, so leave as pedwarn

and later at line 1177
      pedwarn ("name lookup of %qD changed for new ISO %<for%> scoping",
	       DECL_NAME (decl));
      pedwarn ("  using obsolete binding at %q+D", decl);
This is currently inaccurate, it says "using obsolete binding" but
because pedantic-errors is on it doesn't use it - you get a fatal
error.
Leaving this as a pedwarn would make the diagnostic accurate, but
personally I think it's right that this is a fatal error, so it should
change to permerror and the second line should be changed to
   "(if you use -fpermissive G++ will accept your code)"
as used in lex.c and parser.c

The others in cp/name-lookup.c should be pedwarns.

In cp/parser.c your patch already changes one pedwarn.  I think all
others should change unless guarded by if (pedantic), as this allow
GNU extensions like long long.

There are a couple of exceptions...

There's this oddity on line 13685

      if (!parser->default_arg_ok_p)
	{
	  if (!flag_pedantic_errors)
	    warning (0, "deprecated use of default argument for parameter of
non-function");
	  else
	    {
	      error ("default arguments are only permitted for function parameters");
	      default_argument = NULL_TREE;
	    }
	}

I think for consistency with the new proposed behaviour it should
check flag_permissive, instead of !flag_pedantic_errors, do you agree?

on line 14511:
      if (scope == nested_name_specifier)
	{
	  pedwarn ("extra qualification ignored");
	  nested_name_specifier = NULL_TREE;
	  num_templates = 0;
	}
how confusing - a fatal error that says the problem was ignored :-)
I think this should be a permerror, but maybe the text should say
"extra qualification not allowed"

The other pedwarns in cp/parser.c that are not guarded by pedantic
should be permerrors.

In cp/pt.c all pedwarns except these should be permerrors:

line 14284
      if (pedantic && !in_system_header)
	pedwarn ("ISO C++ forbids the use of %<extern%> on explicit "
		 "instantiations");
This is a GNU extension, leave as pedwarn
line 14370
      if (pedantic && !in_system_header)
	pedwarn("ISO C++ forbids the use of %qE on explicit instantiations",
		storage);
GNU extension, leave as pedwarn

The others in cp/pt.c should be permerrors.

The pedwarn in cp/semantics.c should be a permerror

In cp/typeck2.c I think cxx_incomplete_type_diagnostic should continue
to use pedwarn on line 347, but line 697 should be a permerror.

In cp/typeck.c the pedwarns guarded by pedantic should stay as pedwarns.
on line 5356 there is:
      if (pedantic)
	/* Only issue a warning, as we have always supported this
	   where possible, and it is necessary in some cases.  DR 195
	   addresses this issue, but as of 2004/10/26 is still in
	   drafting.  */
	warning (0, "ISO C++ forbids casting between pointer-to-function and
pointer-to-object");

This could switch to a pedwarn, since that will still be a warning
under the permerror proposal. That would mean it can be turned into a
fatal error with -pedantic-errors instead of -Werror, which would be
consistent with other warnings that are only enabled by -pedantic.
I'm not sure how "conditionally-supported behaviour" should interact
with -pedantic-errors so let's leave as a warning for now.

The others (including the one in build_x_compound_expr_from_list)
should change to permerrors

Is that all of them?  Would you prefer a patch to make the changes or
are you going to go through them yourself anyway?

Jon

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Manuel López-Ibáñez]

On 14/01/2008, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 13/01/2008, Jonathan Wakely wrote:
> >
> > I think all others should change to permerrors.
>
> I only meant all others in cp/decl.c of course - here are the remaining files.
> Again I've only listed the ones that should remain as pedwarns and
> other special cases - most change to permerrors.
>

Thanks for doing this!

> Another interesting one.  Variadic templates work in C++98 mode and
> are used internally, but currently you have to say -fpermissive to use
> them in user code. That flag is *usually* needed for
> backward-compatibility, not enabling *future* extensions.

That is because we didn't have permerror and the semantics of
fpermissive were not clear. Now it is evident that this should be a
pedwarn. Wow, you found probably the best example of why the current
C++ FE behaviour is confusing.

> So is support for variadic templates in -std=gnu98 mode an official
> extension, or a detail not-for-public-consumption?

Well, right now you can use it if you use fpermissive, so if it wasn't
for public-consumption, using pedwarn instead of error was not the
right thing in the first place. So pedwarn.

> I think for consistency with the new proposed behaviour it should
> check flag_permissive, instead of !flag_pedantic_errors, do you agree?
>

Yes, this is the type of thing I am looking when reviewing your
suggestions, since they are a potential source of bugs. In my patch I
changed a few.

> In cp/typeck.c the pedwarns guarded by pedantic should stay as pedwarns.
> on line 5356 there is:
>       if (pedantic)
>         /* Only issue a warning, as we have always supported this
>            where possible, and it is necessary in some cases.  DR 195
>            addresses this issue, but as of 2004/10/26 is still in
>            drafting.  */
>         warning (0, "ISO C++ forbids casting between pointer-to-function and
> pointer-to-object");
>
> This could switch to a pedwarn, since that will still be a warning
> under the permerror proposal. That would mean it can be turned into a
> fatal error with -pedantic-errors instead of -Werror, which would be
> consistent with other warnings that are only enabled by -pedantic.
> I'm not sure how "conditionally-supported behaviour" should interact
> with -pedantic-errors so let's leave as a warning for now.

This is another example of trying to workaround the wrong default of
pedwarns. This should be a pedwarn. The reason why is a warning is
that pedantic-errors was active by default and that didn't make sense.
We are changing this, so we don't need this hack anymore.

> Is that all of them?  Would you prefer a patch to make the changes or
> are you going to go through them yourself anyway?

I agree with all your other suggestions. I will try to go through them
in the following days and submit a follow-up patch to the ones already
submitted.

Many thanks! That was impressive.

Manuel.

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Paolo Bonzini]

Ismail Dönmez wrote:
> Sunday 13 January 2008 18:03:20 tarihinde Andreas Schwab şunları yazmıştı:
>> Ismail Dönmez <ismail@pardus.org.tr> writes:
>>> That was just an example, real life testcase shows that problem stems
>>> from autoconf and its config.h. Projects end up defining things like
>>> HAVE_STDLIB_H twice which is not harmful at all but now causes an error
>>> if g++ is used.
>> Redefinitions with the same replacement list are not an error.
> 
> Ok but that still doesn't cover the general use of
> 
> #define PACKAGE_NAME foobar

We had to fix it ourselves for libffi as part of switching to autoconf 
2.50+.  The point is that installing config.h headers is broken by 
design -- and this is not something that the GCC community states.  See 
for example

http://www.cygwin.com/ml/automake/2000-10/msg00116.html

"Unfortunately, people are being lulled into a false sense of security,
because installing config.h appears to work while it is an uncommon
practice.  Things will go downhill fast if this becomes common
practice -- imagine the chaos that we would have if gtk.h indirectly
#included a raw (i.e. with rewriting prior to installation) config.h,
which set up a whole bunch of HAVE_foo macros and VERSION's that you
don't want to cope with when you use libgtk.  Even worse what if the
gnome headers installed another conig.h with overlapping and
contradicting macros...."

which warned about this already 8 years ago.  Sweet.

Why not fixing the handful of packages with a /^#define PACKAGE/d, 
instead of adding -fpermissive to the 50 users of those broken packages?
Paolo

Re: Changes in C++ FE regarding pedwarns to be errors are harmful

[Ismail Dönmez]

Monday 14 January 2008 12:34:03 tarihinde Paolo Bonzini şunları yazmıştı:
> Why not fixing the handful of packages with a /^#define PACKAGE/d,
> instead of adding -fpermissive to the 50 users of those broken packages?

That simple fix won't work, there might be installed headers which depend on 
definitions in config.h . This might be broken but there are just too many 
cases to fix. At least gcc should have warned in gcc 4.3 that this will be an 
error in next release and do this in 4.4 instead of breaking stuff en masse.

Regards,
ismail


-- 
Never learn by your mistakes, if you do you may never dare to try again.