Warning annoyances in list_read.c

Warning annoyances in list_read.c

[Jerry DeLisle]

See: https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html

I tried this by placing these at the beginning and end of the file:

diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c
index 39805baa..58b729a1 100644
--- a/libgfortran/io/list_read.c
+++ b/libgfortran/io/list_read.c
@@ -73,6 +73,9 @@ typedef unsigned char uchar;
 #define next_char(dtp) ((dtp)->u.p.current_unit->next_char_fn_ptr (dtp))
 #define push_char(dtp, c) ((dtp)->u.p.current_unit->push_char_fn_ptr (dtp, c))

+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wimplicit-fallthrough"
+
 /* Worker function to save a default KIND=1 character to a string
    buffer, enlarging it as necessary.  */

@@ -3615,3 +3625,5 @@ nml_err_ret:
   generate_error (&dtp->common, LIBERROR_READ_VALUE, nml_err_msg);
   return;
 }
+
+#pragma GCC diagnostic pop

Any objections to doing this?


Maybe this is another way to deal with some of the other annoying warnings that
seem to be showing up.

Let me know what people think.

Jerry

Re: Warning annoyances in list_read.c

[Steve Kargl]

On Sun, Mar 26, 2017 at 11:27:59AM -0700, Jerry DeLisle wrote:
> 
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wimplicit-fallthrough"

IMNSHO, the correct fix is to complain loudly to whomever
added -Wimplicit-fallthrough to compiler options.  It should
be removed (especially if is has been added to -Wall).

You can also probably add -Wno-implicit-fallthrough to 
libgfortran/configure.ac at 

# Add -Wall -fno-repack-arrays -fno-underscoring if we are using GCC.
if test "x$GCC" = "xyes"; then
  AM_FCFLAGS="-I . -Wall -Werror -fimplicit-none -fno-repack-arrays -fno-underscoring"

-- 
Steve
20161221 https://www.youtube.com/watch?v=IbCHE-hONow

Re: Warning annoyances in list_read.c

[Jerry DeLisle]

On 03/26/2017 11:45 AM, Steve Kargl wrote:
> On Sun, Mar 26, 2017 at 11:27:59AM -0700, Jerry DeLisle wrote:
>>
>> +#pragma GCC diagnostic push
>> +#pragma GCC diagnostic ignored "-Wimplicit-fallthrough"
> 
> IMNSHO, the correct fix is to complain loudly to whomever
> added -Wimplicit-fallthrough to compiler options.  It should
> be removed (especially if is has been added to -Wall).
> 
> You can also probably add -Wno-implicit-fallthrough to 
> libgfortran/configure.ac at 
> 
> # Add -Wall -fno-repack-arrays -fno-underscoring if we are using GCC.
> if test "x$GCC" = "xyes"; then
>   AM_FCFLAGS="-I . -Wall -Werror -fimplicit-none -fno-repack-arrays -fno-underscoring"
> 

Problem I have is I don't know who to complain to. I think there is a bit of a
glass wall going on here anyway, so what would be the point of complaining if
the retrievers of the message all have the ON-OFF switch in the OFF position.
(After all, I do not have a PHD, I am not a computer science graduate, why
bother looking down ones nose at a low life such as myself, OMG its an engineer,
what the hell does he know.)

Maybe these warnings are being turned on as a matter of policy, but truth is,
when I build 50 times a day, the warnings flying by are masking the errors or
other warnings that may be important. For example, I inadvertently passed a ptr
to a function rather than the *ptr.

The warning that ensued flew by mixed in with all the other crap warnings and I
did not see it. That cost me wasted cycle time (remember, I am not an expert and
should not be expected to see such things. Hell, for that matter I should not
even be doing any of this work. :)

Cheers everybody, its been dark and gray all day.

Jerry

Re: Warning annoyances in list_read.c

[Steve Kargl]

On Sun, Mar 26, 2017 at 06:45:07PM -0700, Jerry DeLisle wrote:
> On 03/26/2017 11:45 AM, Steve Kargl wrote:
> > On Sun, Mar 26, 2017 at 11:27:59AM -0700, Jerry DeLisle wrote:
> >>
> >> +#pragma GCC diagnostic push
> >> +#pragma GCC diagnostic ignored "-Wimplicit-fallthrough"
> > 
> > IMNSHO, the correct fix is to complain loudly to whomever
> > added -Wimplicit-fallthrough to compiler options.  It should
> > be removed (especially if is has been added to -Wall).
> > 
> > You can also probably add -Wno-implicit-fallthrough to 
> > libgfortran/configure.ac at 
> > 
> > # Add -Wall -fno-repack-arrays -fno-underscoring if we are using GCC.
> > if test "x$GCC" = "xyes"; then
> >   AM_FCFLAGS="-I . -Wall -Werror -fimplicit-none -fno-repack-arrays -fno-underscoring"
> > 
> 
> Problem I have is I don't know who to complain to. I think there is a bit of a
> glass wall going on here anyway, so what would be the point of complaining if
> the retrievers of the message all have the ON-OFF switch in the OFF position.
> (After all, I do not have a PHD, I am not a computer science graduate, why
> bother looking down ones nose at a low life such as myself, OMG its an engineer,
> what the hell does he know.)
> 
> Maybe these warnings are being turned on as a matter of policy, but truth is,
> when I build 50 times a day, the warnings flying by are masking the errors or
> other warnings that may be important. For example, I inadvertently passed a ptr
> to a function rather than the *ptr.
> 
> The warning that ensued flew by mixed in with all the other crap warnings and I
> did not see it. That cost me wasted cycle time (remember, I am not an expert and
> should not be expected to see such things. Hell, for that matter I should not
> even be doing any of this work. :)
> 

This option is clearly enforceing someone's preferred markup of
adding a comment to explicitly note a fall through.  Candidate
individual to complain to

2016-09-26  Marek Polacek  <polacek@redhat.com>

        PR c/7652
        * common.opt (Wimplicit-fallthrough): New option.
        * doc/extend.texi: Document statement attributes and the fallthrough
        attribute.
        * doc/invoke.texi: Document -Wimplicit-fallthrough.
        * gimple.h (gimple_call_internal_p): New function.
        * gimplify.c (struct gimplify_ctx): Add in_switch_expr.
        (struct label_entry): New struct.
        (find_label_entry): New function.
        (case_label_p): New function.
        (collect_fallthrough_labels): New function.
        (last_stmt_in_scope): New function.
        (should_warn_for_implicit_fallthrough): New function.
        (warn_implicit_fallthrough_r): New function.
        (maybe_warn_implicit_fallthrough): New function.
        (expand_FALLTHROUGH_r): New function.
        (expand_FALLTHROUGH): New function.
        (gimplify_switch_expr): Call maybe_warn_implicit_fallthrough and
        expand_FALLTHROUGH for the innermost GIMPLE_SWITCH.
        (gimplify_label_expr): New function.
        (gimplify_case_label_expr): Set location.
        (gimplify_expr): Call gimplify_label_expr.
        * internal-fn.c (expand_FALLTHROUGH): New function.
        * internal-fn.def (FALLTHROUGH): New internal function.
        * langhooks.c (lang_GNU_OBJC): New function.
        * langhooks.h (lang_GNU_OBJC): Declare.
        * system.h (gcc_fallthrough): Define.
        * tree-core.h: Add FALLTHROUGH_LABEL_P comment.
        * tree.h (FALLTHROUGH_LABEL_P): Define.

If he added a new option affecting libgfortran, then he should
fix up libgfortran.

-- 
Steve
20161221 https://www.youtube.com/watch?v=IbCHE-hONow

Re: Warning annoyances in list_read.c

[Markus Trippelsdorf]

On 2017.03.26 at 19:30 -0700, Steve Kargl wrote:
> On Sun, Mar 26, 2017 at 06:45:07PM -0700, Jerry DeLisle wrote:
> > On 03/26/2017 11:45 AM, Steve Kargl wrote:
> > > On Sun, Mar 26, 2017 at 11:27:59AM -0700, Jerry DeLisle wrote:
> > >>
> > >> +#pragma GCC diagnostic push
> > >> +#pragma GCC diagnostic ignored "-Wimplicit-fallthrough"
> > > 
> > > IMNSHO, the correct fix is to complain loudly to whomever
> > > added -Wimplicit-fallthrough to compiler options.  It should
> > > be removed (especially if is has been added to -Wall).
> > > 
> > > You can also probably add -Wno-implicit-fallthrough to 
> > > libgfortran/configure.ac at 
> > > 
> > > # Add -Wall -fno-repack-arrays -fno-underscoring if we are using GCC.
> > > if test "x$GCC" = "xyes"; then
> > >   AM_FCFLAGS="-I . -Wall -Werror -fimplicit-none -fno-repack-arrays -fno-underscoring"
> > > 
> > 
> > Problem I have is I don't know who to complain to. I think there is a bit of a
> > glass wall going on here anyway, so what would be the point of complaining if
> > the retrievers of the message all have the ON-OFF switch in the OFF position.
> > (After all, I do not have a PHD, I am not a computer science graduate, why
> > bother looking down ones nose at a low life such as myself, OMG its an engineer,
> > what the hell does he know.)
> > 
> > Maybe these warnings are being turned on as a matter of policy, but truth is,
> > when I build 50 times a day, the warnings flying by are masking the errors or
> > other warnings that may be important. For example, I inadvertently passed a ptr
> > to a function rather than the *ptr.
> > 
> > The warning that ensued flew by mixed in with all the other crap warnings and I
> > did not see it. That cost me wasted cycle time (remember, I am not an expert and
> > should not be expected to see such things. Hell, for that matter I should not
> > even be doing any of this work. :)
> > 
> 
> This option is clearly enforceing someone's preferred markup of
> adding a comment to explicitly note a fall through.  Candidate
> individual to complain to
> 
> If he added a new option affecting libgfortran, then he should
> fix up libgfortran.

He didn't add the warning to specifically annoy fortran developers.
It is trivial to add seven gcc_fallthrough() or breaks for someone who
knows the code and the person who added the warning obviously doesn't.

-- 
Markus

Re: Warning annoyances in list_read.c

[Steve Kargl]

On Mon, Mar 27, 2017 at 08:58:43AM +0200, Markus Trippelsdorf wrote:
> On 2017.03.26 at 19:30 -0700, Steve Kargl wrote:
> > On Sun, Mar 26, 2017 at 06:45:07PM -0700, Jerry DeLisle wrote:
> > > On 03/26/2017 11:45 AM, Steve Kargl wrote:
> > > > On Sun, Mar 26, 2017 at 11:27:59AM -0700, Jerry DeLisle wrote:
> > > >>
> > > >> +#pragma GCC diagnostic push
> > > >> +#pragma GCC diagnostic ignored "-Wimplicit-fallthrough"
> > > > 
> > > > IMNSHO, the correct fix is to complain loudly to whomever
> > > > added -Wimplicit-fallthrough to compiler options.  It should
> > > > be removed (especially if is has been added to -Wall).
> > > > 
> > > > You can also probably add -Wno-implicit-fallthrough to 
> > > > libgfortran/configure.ac at 
> > > > 
> > > > # Add -Wall -fno-repack-arrays -fno-underscoring if we are using GCC.
> > > > if test "x$GCC" = "xyes"; then
> > > >   AM_FCFLAGS="-I . -Wall -Werror -fimplicit-none -fno-repack-arrays -fno-underscoring"
> > > > 
> > > 
> > > Problem I have is I don't know who to complain to. I think there is a bit of a
> > > glass wall going on here anyway, so what would be the point of complaining if
> > > the retrievers of the message all have the ON-OFF switch in the OFF position.
> > > (After all, I do not have a PHD, I am not a computer science graduate, why
> > > bother looking down ones nose at a low life such as myself, OMG its an engineer,
> > > what the hell does he know.)
> > > 
> > > Maybe these warnings are being turned on as a matter of policy, but truth is,
> > > when I build 50 times a day, the warnings flying by are masking the errors or
> > > other warnings that may be important. For example, I inadvertently passed a ptr
> > > to a function rather than the *ptr.
> > > 
> > > The warning that ensued flew by mixed in with all the other crap warnings and I
> > > did not see it. That cost me wasted cycle time (remember, I am not an expert and
> > > should not be expected to see such things. Hell, for that matter I should not
> > > even be doing any of this work. :)
> > > 
> > 
> > This option is clearly enforceing someone's preferred markup of
> > adding a comment to explicitly note a fall through.  Candidate
> > individual to complain to
> > 
> > If he added a new option affecting libgfortran, then he should
> > fix up libgfortran.
> 
> He didn't add the warning to specifically annoy fortran developers.
> It is trivial to add seven gcc_fallthrough() or breaks for someone who
> knows the code and the person who added the warning obviously doesn't.
> 

I completely disagree with your viewpoint here.  If someone turns
on a silly warning, that someone should fix all places within the
tree that triggers that warning.  There is ZERO value to this warning,
but added work for others to clean up that someone's mess.

-- 
Steve
20161221 https://www.youtube.com/watch?v=IbCHE-hONow

Re: Warning annoyances in list_read.c

[Jonathan Wakely]

On 27 March 2017 at 14:26, Steve Kargl wrote:
> I completely disagree with your viewpoint here.  If someone turns
> on a silly warning, that someone should fix all places within the
> tree that triggers that warning.  There is ZERO value to this warning,
> but added work for others to clean up that someone's mess.

Your absolutist view is just an opinion and reasonable people disagree
on the value of the warning. It's already found bugs in real code.

You could continue being upset, or somebody who understands the code
could just fix the warnings and everybody can get on with their lives.

Re: Warning annoyances in list_read.c

[Markus Trippelsdorf]

On 2017.03.27 at 06:26 -0700, Steve Kargl wrote:
> On Mon, Mar 27, 2017 at 08:58:43AM +0200, Markus Trippelsdorf wrote:
> > On 2017.03.26 at 19:30 -0700, Steve Kargl wrote:
> > > On Sun, Mar 26, 2017 at 06:45:07PM -0700, Jerry DeLisle wrote:
> > > > On 03/26/2017 11:45 AM, Steve Kargl wrote:
> > > > > On Sun, Mar 26, 2017 at 11:27:59AM -0700, Jerry DeLisle wrote:
> > > > >>
> > > > >> +#pragma GCC diagnostic push
> > > > >> +#pragma GCC diagnostic ignored "-Wimplicit-fallthrough"
> > > > > 
> > > > > IMNSHO, the correct fix is to complain loudly to whomever
> > > > > added -Wimplicit-fallthrough to compiler options.  It should
> > > > > be removed (especially if is has been added to -Wall).
> > > > > 
> > > > > You can also probably add -Wno-implicit-fallthrough to 
> > > > > libgfortran/configure.ac at 
> > > > > 
> > > > > # Add -Wall -fno-repack-arrays -fno-underscoring if we are using GCC.
> > > > > if test "x$GCC" = "xyes"; then
> > > > >   AM_FCFLAGS="-I . -Wall -Werror -fimplicit-none -fno-repack-arrays -fno-underscoring"
> > > > > 
> > > > 
> > > > Problem I have is I don't know who to complain to. I think there is a bit of a
> > > > glass wall going on here anyway, so what would be the point of complaining if
> > > > the retrievers of the message all have the ON-OFF switch in the OFF position.
> > > > (After all, I do not have a PHD, I am not a computer science graduate, why
> > > > bother looking down ones nose at a low life such as myself, OMG its an engineer,
> > > > what the hell does he know.)
> > > > 
> > > > Maybe these warnings are being turned on as a matter of policy, but truth is,
> > > > when I build 50 times a day, the warnings flying by are masking the errors or
> > > > other warnings that may be important. For example, I inadvertently passed a ptr
> > > > to a function rather than the *ptr.
> > > > 
> > > > The warning that ensued flew by mixed in with all the other crap warnings and I
> > > > did not see it. That cost me wasted cycle time (remember, I am not an expert and
> > > > should not be expected to see such things. Hell, for that matter I should not
> > > > even be doing any of this work. :)
> > > > 
> > > 
> > > This option is clearly enforceing someone's preferred markup of
> > > adding a comment to explicitly note a fall through.  Candidate
> > > individual to complain to
> > > 
> > > If he added a new option affecting libgfortran, then he should
> > > fix up libgfortran.
> > 
> > He didn't add the warning to specifically annoy fortran developers.
> > It is trivial to add seven gcc_fallthrough() or breaks for someone who
> > knows the code and the person who added the warning obviously doesn't.
> > 
> 
> I completely disagree with your viewpoint here.  If someone turns
> on a silly warning, that someone should fix all places within the
> tree that triggers that warning.  There is ZERO value to this warning,
> but added work for others to clean up that someone's mess.

Well, a missing break is a bug. No?
This warning has already uncovered several bugs in the tree, so calling
it silly makes no sense.

-- 
Markus

Re: Warning annoyances in list_read.c

[Steve Kargl]

On Mon, Mar 27, 2017 at 02:36:27PM +0100, Jonathan Wakely wrote:
> On 27 March 2017 at 14:26, Steve Kargl wrote:
> > I completely disagree with your viewpoint here.  If someone turns
> > on a silly warning, that someone should fix all places within the
> > tree that triggers that warning.  There is ZERO value to this warning,
> > but added work for others to clean up that someone's mess.
> 
> Your absolutist view is just an opinion and reasonable people disagree
> on the value of the warning. It's already found bugs in real code.
> 
> You could continue being upset, or somebody who understands the code
> could just fix the warnings and everybody can get on with their lives.

Go scan the gcc-patches mailing list for "fallthrough".  I'll
note other have concerns.  Here's one example:

https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00300.html

   Without Bernd's patch to set the default to 1 you will drown
   in false positives once you start using gcc-7 to build a whole
   distro. On my Gentoo test box anything but level 1 is simply
   unacceptable, because you will miss important other warnings
   in the -Wimplicit-fallthrough noise otherwise.

The code is valid C.  So, you'll fixing already valid code.

-- 
Steve
20161221 https://www.youtube.com/watch?v=IbCHE-hONow

Re: Warning annoyances in list_read.c

[Steve Kargl]

On Mon, Mar 27, 2017 at 03:39:37PM +0200, Markus Trippelsdorf wrote:
> 
> Well, a missing break is a bug. No?

Every 'case' statement without exception must be accompanied by
a 'break' statement?  Wasting others' time to "fix" working
correct code is acceptable?

-- 
Steve
20161221 https://www.youtube.com/watch?v=IbCHE-hONow

Re: Warning annoyances in list_read.c

[Markus Trippelsdorf]

On 2017.03.27 at 07:44 -0700, Steve Kargl wrote:
> On Mon, Mar 27, 2017 at 03:39:37PM +0200, Markus Trippelsdorf wrote:
> > 
> > Well, a missing break is a bug. No?
> 
> Every 'case' statement without exception must be accompanied by
> a 'break' statement?  Wasting others' time to "fix" working
> correct code is acceptable?

Sorry, I should have written "potential bug".
For legacy code I would simply disable the warning.
But to dismiss it utterly, as you do, is shortsighted, because it has
the potential to point out real bugs.

-- 
Markus

Re: Warning annoyances in list_read.c

[Jonathan Wakely]

On 27 March 2017 at 14:49, Steve Kargl wrote:
> On Mon, Mar 27, 2017 at 02:36:27PM +0100, Jonathan Wakely wrote:
>> On 27 March 2017 at 14:26, Steve Kargl wrote:
>> > I completely disagree with your viewpoint here.  If someone turns
>> > on a silly warning, that someone should fix all places within the
>> > tree that triggers that warning.  There is ZERO value to this warning,
>> > but added work for others to clean up that someone's mess.
>>
>> Your absolutist view is just an opinion and reasonable people disagree
>> on the value of the warning. It's already found bugs in real code.
>>
>> You could continue being upset, or somebody who understands the code
>> could just fix the warnings and everybody can get on with their lives.
>
> Go scan the gcc-patches mailing list for "fallthrough".  I'll
> note other have concerns.  Here's one example:
>
> https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00300.html
>
>    Without Bernd's patch to set the default to 1 you will drown
>    in false positives once you start using gcc-7 to build a whole
>    distro. On my Gentoo test box anything but level 1 is simply
>    unacceptable, because you will miss important other warnings
>    in the -Wimplicit-fallthrough noise otherwise.
>
> The code is valid C.  So, you'll fixing already valid code.

That's the case for many of GCC's warnings, as you are surely aware.
Please re-read the documentation for -Wall:

This enables all the warnings about constructions that some users
consider questionable, and that are easy to avoid (or modify to
prevent the warning), even in conjunction with macros. [...] Note that
some warning flags are not implied by -Wall.  Some of them warn about
constructions that users generally do not consider questionable, but
which occasionally you might wish to check for; others warn about
constructions that are necessary or hard to avoid in some cases, and
there is no simple way to modify the code to suppress the warning.
Some of them are enabled by -Wextra but many of them must be enabled
individually.

-Wimplicit-fallthrough is in the latter category, despite it being
easy to modify the code to avoid the warning.

Re: Warning annoyances in list_read.c

[Jonathan Wakely]

On 27 March 2017 at 14:49, Steve Kargl wrote:
> On Mon, Mar 27, 2017 at 02:36:27PM +0100, Jonathan Wakely wrote:
>> On 27 March 2017 at 14:26, Steve Kargl wrote:
>> > I completely disagree with your viewpoint here.  If someone turns
>> > on a silly warning, that someone should fix all places within the
>> > tree that triggers that warning.  There is ZERO value to this warning,
>> > but added work for others to clean up that someone's mess.
>>
>> Your absolutist view is just an opinion and reasonable people disagree
>> on the value of the warning. It's already found bugs in real code.
>>
>> You could continue being upset, or somebody who understands the code
>> could just fix the warnings and everybody can get on with their lives.
>
> Go scan the gcc-patches mailing list for "fallthrough".  I'll
> note other have concerns.  Here's one example:

Also "other people have concerns" doesn't refute that it's just an
opinion, and not everybody shares that opinion.

"THIS IS WRONG, YOU MUST FIX IT" is unproductive. Somebody could
disable -Wimplicit-fallthrough for the Fortran front-end, or add the
fallthrough comments, and stop wasting the time of everybody who reads
the gcc@ list. If the code is 100% correct and there is no chance of
accidentally omitted break statements in the Fortran front-end then
just disable the warning, it's not meant for infallible programmers.

Re: Warning annoyances in list_read.c

[Markus Trippelsdorf]

On 2017.03.27 at 06:49 -0700, Steve Kargl wrote:
> On Mon, Mar 27, 2017 at 02:36:27PM +0100, Jonathan Wakely wrote:
> > On 27 March 2017 at 14:26, Steve Kargl wrote:
> > > I completely disagree with your viewpoint here.  If someone turns
> > > on a silly warning, that someone should fix all places within the
> > > tree that triggers that warning.  There is ZERO value to this warning,
> > > but added work for others to clean up that someone's mess.
> > 
> > Your absolutist view is just an opinion and reasonable people disagree
> > on the value of the warning. It's already found bugs in real code.
> > 
> > You could continue being upset, or somebody who understands the code
> > could just fix the warnings and everybody can get on with their lives.
> 
> Go scan the gcc-patches mailing list for "fallthrough".  I'll
> note other have concerns.  Here's one example:
> 
> https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00300.html
> 
>    Without Bernd's patch to set the default to 1 you will drown
>    in false positives once you start using gcc-7 to build a whole
>    distro. On my Gentoo test box anything but level 1 is simply
>    unacceptable, because you will miss important other warnings
>    in the -Wimplicit-fallthrough noise otherwise.

The quotation doesn't have anything to do with the current discussion,
which is the general usefulness of the warning.
It only talks about one of the (admittedly over-engineered) six
different levels of the warning.

-- 
Markus

Re: Warning annoyances in list_read.c

[Steve Kargl]

On Mon, Mar 27, 2017 at 05:22:12PM +0200, Markus Trippelsdorf wrote:
> On 2017.03.27 at 06:49 -0700, Steve Kargl wrote:
> > 
> > Go scan the gcc-patches mailing list for "fallthrough".  I'll
> > note other have concerns.  Here's one example:
> > 
> > https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00300.html
> > 
> >    Without Bernd's patch to set the default to 1 you will drown
> >    in false positives once you start using gcc-7 to build a whole
> >    distro. On my Gentoo test box anything but level 1 is simply
> >    unacceptable, because you will miss important other warnings
> >    in the -Wimplicit-fallthrough noise otherwise.
> 
> The quotation doesn't have anything to do with the current discussion,
> which is the general usefulness of the warning.
> It only talks about one of the (admittedly over-engineered) six
> different levels of the warning.
> 

Yes, it does.  See the part about "... drown in false positives ..."
Whoever turned this option on should have been prepared to deal
with the fallout by investigating each and every warning (i.e.,
either fix a real bug or (un)fix valid code to prevent the false
positive).  

But that's okay.  I now understand that it is acceptable for
a developer to commit a change that causes issues for other
developers, and said developer can turn a blind eye.

-- 
Steve
20161221 https://www.youtube.com/watch?v=IbCHE-hONow

Re: Warning annoyances in list_read.c

[Marek Polacek]

On Mon, Mar 27, 2017 at 09:27:34AM -0700, Steve Kargl wrote:
> On Mon, Mar 27, 2017 at 05:22:12PM +0200, Markus Trippelsdorf wrote:
> > On 2017.03.27 at 06:49 -0700, Steve Kargl wrote:
> > > 
> > > Go scan the gcc-patches mailing list for "fallthrough".  I'll
> > > note other have concerns.  Here's one example:
> > > 
> > > https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00300.html
> > > 
> > >    Without Bernd's patch to set the default to 1 you will drown
> > >    in false positives once you start using gcc-7 to build a whole
> > >    distro. On my Gentoo test box anything but level 1 is simply
> > >    unacceptable, because you will miss important other warnings
> > >    in the -Wimplicit-fallthrough noise otherwise.
> > 
> > The quotation doesn't have anything to do with the current discussion,
> > which is the general usefulness of the warning.
> > It only talks about one of the (admittedly over-engineered) six
> > different levels of the warning.
> > 
> 
> Yes, it does.  See the part about "... drown in false positives ..."
> Whoever turned this option on should have been prepared to deal
> with the fallout by investigating each and every warning (i.e.,
> either fix a real bug or (un)fix valid code to prevent the false
> positive).  
 
Having spent hours on fixing various fallthrough cases throughout the codebase,
deciding whether or not a particular case is an intentional fallthru, and
pursuing various maintainers if I couldn't make a call, I find your statement,
erm, incorrect .  I'm sorry that apparently something has slipped through.
I would've fixed it if I'd hit it.

The warning had been discussed extensively on the ML, and you had the chance to
chime in, too.  There's a reason why the warning is only enabled by -Wextra and
not by -Wall.

> But that's okay.  I now understand that it is acceptable for
> a developer to commit a change that causes issues for other
> developers, and said developer can turn a blind eye.

Nonsense.

	Marek

Re: Warning annoyances in list_read.c

[Steve Kargl]

On Mon, Mar 27, 2017 at 06:45:32PM +0200, Marek Polacek wrote:
> On Mon, Mar 27, 2017 at 09:27:34AM -0700, Steve Kargl wrote:
> > On Mon, Mar 27, 2017 at 05:22:12PM +0200, Markus Trippelsdorf wrote:
> > > On 2017.03.27 at 06:49 -0700, Steve Kargl wrote:
> > > > 
> > > > Go scan the gcc-patches mailing list for "fallthrough".  I'll
> > > > note other have concerns.  Here's one example:
> > > > 
> > > > https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00300.html
> > > > 
> > > >    Without Bernd's patch to set the default to 1 you will drown
> > > >    in false positives once you start using gcc-7 to build a whole
> > > >    distro. On my Gentoo test box anything but level 1 is simply
> > > >    unacceptable, because you will miss important other warnings
> > > >    in the -Wimplicit-fallthrough noise otherwise.
> > > 
> > > The quotation doesn't have anything to do with the current discussion,
> > > which is the general usefulness of the warning.
> > > It only talks about one of the (admittedly over-engineered) six
> > > different levels of the warning.
> > > 
> > 
> > Yes, it does.  See the part about "... drown in false positives ..."
> > Whoever turned this option on should have been prepared to deal
> > with the fallout by investigating each and every warning (i.e.,
> > either fix a real bug or (un)fix valid code to prevent the false
> > positive).  
>  
> Having spent hours on fixing various fallthrough cases throughout the codebase,
> deciding whether or not a particular case is an intentional fallthru, and
> pursuing various maintainers if I couldn't make a call, I find your statement,
> erm, incorrect .  I'm sorry that apparently something has slipped through.
> I would've fixed it if I'd hit it.

A grep of the ChangeLog* files in libgfortran suggests that 
whoever turned on this option made no effort with regards to
libgfortran.

% pwd
/usr/home/sgk/gcc/gcc7/libgfortran
% grep -i marek ChangeLog ChangeLog-2016
% grep -i polacek ChangeLog ChangeLog-2016
% grep -i fall ChangeLog ChangeLog-2016
ChangeLog-2016: (_gfortran_caf_is_present): Prevent fallthrough warnings.
ChangeLog-2016: MinGW-w64. Fix bounds overflow in fallback code.

The _gfortran_caf_is_present change was made by Andre Vehreschild.

-- 
Steve
20161221 https://www.youtube.com/watch?v=IbCHE-hONow

Re: Warning annoyances in list_read.c

[Toon Moene]

On 03/27/2017 06:45 PM, Marek Polacek wrote:

> On Mon, Mar 27, 2017 at 09:27:34AM -0700, Steve Kargl wrote:

>> But that's okay.  I now understand that it is acceptable for
>> a developer to commit a change that causes issues for other
>> developers, and said developer can turn a blind eye.
>
> Nonsense.

The person developing the warning could *at least* have bootstrapped all 
languages and detected, warned and helped the Fortran/Ada/whatever side 
to cope with it.

[ Man, I'm glad we don't have this problem in Fortran-the-language ].

-- 
Toon Moene - e-mail: toon@moene.org - phone: +31 346 214290
Saturnushof 14, 3738 XG  Maartensdijk, The Netherlands
At home: http://moene.org/~toon/; weather: http://moene.org/~hirlam/
Progress of GNU Fortran: http://gcc.gnu.org/wiki/GFortran#news

Re: Warning annoyances in list_read.c

[Marek Polacek]

On Mon, Mar 27, 2017 at 07:33:05PM +0200, Toon Moene wrote:
> On 03/27/2017 06:45 PM, Marek Polacek wrote:
> 
> > On Mon, Mar 27, 2017 at 09:27:34AM -0700, Steve Kargl wrote:
> 
> > > But that's okay.  I now understand that it is acceptable for
> > > a developer to commit a change that causes issues for other
> > > developers, and said developer can turn a blind eye.
> > 
> > Nonsense.
> 
> The person developing the warning could *at least* have bootstrapped all
> languages and detected, warned and helped the Fortran/Ada/whatever side to
> cope with it.

Of course "the person" had bootstrapped and tested all the languages before
adding the warning.  If only any of you bothered to check the fortran/
ChangeLogs:

2016-08-12  Marek Polacek  <polacek@redhat.com>

        PR c/7652
        * decl.c (match_attr_spec): Add FALLTHRU.
        * primary.c (match_arg_list_function): Likewise.
        * resolve.c (resolve_operator): Adjust fall through comment.
        (fixup_charlen): Add FALLTHRU.
        (resolve_allocate_expr): Adjust fall through comment.
        * trans-array.c (gfc_conv_ss_startstride): Add FALLTHRU.
        * trans-intrinsic.c (gfc_conv_intrinsic_len): Adjust fall through
        comment.

2016-09-26  Marek Polacek  <polacek@redhat.com>

        PR c/7652
        * arith.c (eval_intrinsic): Add gcc_fallthrough.
        * frontend-passes.c (optimize_op): Likewise.
        (gfc_expr_walker): Likewise.
        * parse.c (next_fixed): Likewise.
        * primary.c (match_variable): Likewise.
        * trans-array.c: Likewise.
        * trans-expr.c (flatten_array_ctors_without_strlen): Likewise.
        * trans-io.c (transfer_expr): Likewise.

2016-09-20  Marek Polacek  <polacek@redhat.com>

        * trans-intrinsic.c (conv_expr_ref_to_caf_ref): Adjust fall through
        comment.

	Marek

Re: Warning annoyances in list_read.c

[Thomas Koenig]

Am 27.03.2017 um 19:41 schrieb Marek Polacek:

> Of course "the person" had bootstrapped and tested all the languages before
> adding the warning.  If only any of you bothered to check the fortran/
> ChangeLogs:

The problem is with libfortran, which apparently was not tested
(or the problem would have been found and, presumably, dealt with).

So, due to incomplete testing, a regression was caused.  This has
probably happened a few thousand times before, so it is not an
exceptionally big deal.

We should deal with this the same way we deal with other regressions -
fix it or, if anything else fails, roll back the offending patch.
The person who is responsible for the regression should usually take the
lead in fixing it.

Since the fix appears to be rather trivial, I promise to review
any patch that falls into my area of review (fortran, libfortran)
within 48 hours.

Would this be acceptable to everybody (please)?

Regards

	Thomas

Re: Warning annoyances in list_read.c

[Steve Kargl]

On Mon, Mar 27, 2017 at 07:41:12PM +0200, Marek Polacek wrote:
> On Mon, Mar 27, 2017 at 07:33:05PM +0200, Toon Moene wrote:
> > On 03/27/2017 06:45 PM, Marek Polacek wrote:
> > 
> > > On Mon, Mar 27, 2017 at 09:27:34AM -0700, Steve Kargl wrote:
> > 
> > > > But that's okay.  I now understand that it is acceptable for
> > > > a developer to commit a change that causes issues for other
> > > > developers, and said developer can turn a blind eye.
> > > 
> > > Nonsense.
> > 
> > The person developing the warning could *at least* have bootstrapped all
> > languages and detected, warned and helped the Fortran/Ada/whatever side to
> > cope with it.
> 
> Of course "the person" had bootstrapped and tested all the languages before
> adding the warning.  If only any of you bothered to check the fortran/
> ChangeLogs:
> 

fortran/ != libgfortran/

"The person" also failed to post his changes to fortran/
on the fortran@ mailing list.  So, the fortran changes were
likely not reviewed.

-- 
Steve
20161221 https://www.youtube.com/watch?v=IbCHE-hONow

Re: Warning annoyances in list_read.c

[Jerry DeLisle]

Marek,

Flame wars can be a little fun once in a while.

I had previously tried the /* Fall through */ trick and it did not work as
Dominique clarified in his note. I had not tried putting it inside the Macro.
That makes it easy.

The hard part is catching these details/nuances on how things work.

So we'll do this and move on.

Cheers,

Jerry

PS Dominique Feel free to commit your patch

Re: Warning annoyances in list_read.c

[Richard Biener]

On March 27, 2017 7:59:01 PM GMT+02:00, Thomas Koenig <tkoenig@netcologne.de> wrote:
>Am 27.03.2017 um 19:41 schrieb Marek Polacek:
>
>> Of course "the person" had bootstrapped and tested all the languages
>before
>> adding the warning.  If only any of you bothered to check the
>fortran/
>> ChangeLogs:
>
>The problem is with libfortran, which apparently was not tested
>(or the problem would have been found and, presumably, dealt with).
>
>So, due to incomplete testing, a regression was caused.  This has
>probably happened a few thousand times before, so it is not an
>exceptionally big deal.

Well, the issue is that libgfortran seems to be built without -Werror.

>We should deal with this the same way we deal with other regressions -
>fix it or, if anything else fails, roll back the offending patch.
>The person who is responsible for the regression should usually take
>the
>lead in fixing it.
>
>Since the fix appears to be rather trivial, I promise to review
>any patch that falls into my area of review (fortran, libfortran)
>within 48 hours.
>
>Would this be acceptable to everybody (please)?
>
>Regards
>
>	Thomas

Re: Warning annoyances in list_read.c

[Jonathan Wakely]

On 27 March 2017 at 18:59, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Am 27.03.2017 um 19:41 schrieb Marek Polacek:
>
>> Of course "the person" had bootstrapped and tested all the languages
>> before
>> adding the warning.  If only any of you bothered to check the fortran/
>> ChangeLogs:
>
>
> The problem is with libfortran, which apparently was not tested
> (or the problem would have been found and, presumably, dealt with).
>
> So, due to incomplete testing, a regression was caused.  This has
> probably happened a few thousand times before, so it is not an
> exceptionally big deal.
>
> We should deal with this the same way we deal with other regressions -
> fix it or, if anything else fails, roll back the offending patch.
> The person who is responsible for the regression should usually take the
> lead in fixing it.
>
> Since the fix appears to be rather trivial, I promise to review
> any patch that falls into my area of review (fortran, libfortran)
> within 48 hours.
>
> Would this be acceptable to everybody (please)?

Dominique already provided a patch.

Re: Warning annoyances in list_read.c

[Marek Polacek]

On Mon, Mar 27, 2017 at 07:59:01PM +0200, Thomas Koenig wrote:
> Am 27.03.2017 um 19:41 schrieb Marek Polacek:
> 
> > Of course "the person" had bootstrapped and tested all the languages before
> > adding the warning.  If only any of you bothered to check the fortran/
> > ChangeLogs:
> 
> The problem is with libfortran, which apparently was not tested
> (or the problem would have been found and, presumably, dealt with).
 
I always build libgfortran when testing.  The warning was committed
months ago, so it's weird that I'm only hearing about this now.

I would've been happy to fix the warnings if anyone pointed out them to me.
I hadn't know of them until very recently.

> So, due to incomplete testing, a regression was caused.  This has
> probably happened a few thousand times before, so it is not an
> exceptionally big deal.
> 
> We should deal with this the same way we deal with other regressions -
> fix it or, if anything else fails, roll back the offending patch.
> The person who is responsible for the regression should usually take the
> lead in fixing it.
> 
> Since the fix appears to be rather trivial, I promise to review
> any patch that falls into my area of review (fortran, libfortran)
> within 48 hours.

https://gcc.gnu.org/ml/gcc/2017-03/msg00145.html

	Marek

Re: Warning annoyances in list_read.c

[Marek Polacek]

On Mon, Mar 27, 2017 at 11:16:32AM -0700, Steve Kargl wrote:
> On Mon, Mar 27, 2017 at 07:41:12PM +0200, Marek Polacek wrote:
> > On Mon, Mar 27, 2017 at 07:33:05PM +0200, Toon Moene wrote:
> > > On 03/27/2017 06:45 PM, Marek Polacek wrote:
> > > 
> > > > On Mon, Mar 27, 2017 at 09:27:34AM -0700, Steve Kargl wrote:
> > > 
> > > > > But that's okay.  I now understand that it is acceptable for
> > > > > a developer to commit a change that causes issues for other
> > > > > developers, and said developer can turn a blind eye.
> > > > 
> > > > Nonsense.
> > > 
> > > The person developing the warning could *at least* have bootstrapped all
> > > languages and detected, warned and helped the Fortran/Ada/whatever side to
> > > cope with it.
> > 
> > Of course "the person" had bootstrapped and tested all the languages before
> > adding the warning.  If only any of you bothered to check the fortran/
> > ChangeLogs:
> > 
> 
> fortran/ != libgfortran/
 
I'm aware.  But it's unfair to say that I hadn't tested Fortran when I,
actually, had.

> "The person" also failed to post his changes to fortran/
> on the fortran@ mailing list.  So, the fortran changes were
> likely not reviewed.

It's entirely possible that I may have forgotten to CC fortran@, mea culpa;
although I see that this went to @fortran, but was committed as obvious anyway:
https://gcc.gnu.org/ml/fortran/2016-09/msg00113.html

	Marek

Re: Warning annoyances in list_read.c

[Marek Polacek]

On Mon, Mar 27, 2017 at 11:18:46AM -0700, Jerry DeLisle wrote:
> Marek,
> 
> Flame wars can be a little fun once in a while.
> 
> I had previously tried the /* Fall through */ trick and it did not work as
> Dominique clarified in his note. I had not tried putting it inside the Macro.
> That makes it easy.
 
Yes, that's quite unfortunate (but known).  :(

> The hard part is catching these details/nuances on how things work.

Sure.  Too bad nobody brought this up to me.

> So we'll do this and move on.

Wholeheartedly agreed.

> PS Dominique Feel free to commit your patch

Thanks!

	Marek

Re: Warning annoyances in list_read.c

[Dominique d'Humières]

> Le 27 mars 2017 à 20:34, Marek Polacek <polacek@redhat.com> a écrit :
> 
> On Mon, Mar 27, 2017 at 11:18:46AM -0700, Jerry DeLisle wrote:
>> Marek,
>> 
>> Flame wars can be a little fun once in a while.
>> 
>> I had previously tried the /* Fall through */ trick and it did not work as
>> Dominique clarified in his note. I had not tried putting it inside the Macro.
>> That makes it easy.
> 
> Yes, that's quite unfortunate (but known).  :(
> 
>> The hard part is catching these details/nuances on how things work.
> 
> Sure.  Too bad nobody brought this up to me.
> 
>> So we'll do this and move on.
> 
> Wholeheartedly agreed.
> 
>> PS Dominique Feel free to commit your patch
> 
> Thanks!
> 
> 	Marek

Done as revision r246507.

Dominique

Re: Warning annoyances in list_read.c

[Toon Moene]

On 03/27/2017 08:29 PM, Marek Polacek wrote:

> On Mon, Mar 27, 2017 at 11:16:32AM -0700, Steve Kargl wrote:
>> On Mon, Mar 27, 2017 at 07:41:12PM +0200, Marek Polacek wrote:
>>> On Mon, Mar 27, 2017 at 07:33:05PM +0200, Toon Moene wrote:

>>>> The person developing the warning could *at least* have bootstrapped all
>>>> languages and detected, warned and helped the Fortran/Ada/whatever side to
>>>> cope with it.
>>>
>>> Of course "the person" had bootstrapped and tested all the languages before
>>> adding the warning.  If only any of you bothered to check the fortran/
>>> ChangeLogs:
>>>
>>
>> fortran/ != libgfortran/
>
> I'm aware.  But it's unfair to say that I hadn't tested Fortran when I,
> actually, had.

OK - I see that building libgfortran without -Werror didn't help you 
here (having to manually go through all the logs), so I retract my comment.

My excuses.

-- 
Toon Moene - e-mail: toon@moene.org - phone: +31 346 214290
Saturnushof 14, 3738 XG  Maartensdijk, The Netherlands
At home: http://moene.org/~toon/; weather: http://moene.org/~hirlam/
Progress of GNU Fortran: http://gcc.gnu.org/wiki/GFortran#news

Re: Warning annoyances in list_read.c

[Marek Polacek]

On Mon, Mar 27, 2017 at 12:18:17PM +0200, Dominique d'Humières wrote:
> > > If he added a new option affecting libgfortran, then he should
> > > fix up libgfortran.
> >
> > He didn't add the warning to specifically annoy fortran developers.
> > It is trivial to add seven gcc_fallthrough() or breaks for someone who
> > knows the code and the person who added the warning obviously doesn't.
> The following patch fixes the warnings
> 
> --- ../_clean/libgfortran/io/list_read.c	2017-03-25 20:42:40.000000000 +0100
> +++ libgfortran/io/list_read.c	2017-03-27 12:06:10.000000000 +0200
> @@ -51,7 +51,8 @@ typedef unsigned char uchar;
>  #define CASE_DIGITS   case '0': case '1': case '2': case '3': case '4': \
>                        case '5': case '6': case '7': case '8': case '9'
>  
> -#define CASE_SEPARATORS case ' ': case ',': case '/': case '\n': \
> +#define CASE_SEPARATORS /* Fall through. */ \
> +	case ' ': case ',': case '/': case '\n': \
>  	case '\t': case '\r': case ';'
>  
>  /* This macro assumes that we're operating on a variable.  */
> 
> Indeed before applying this patch, someone will have to check that the warnings do not occur because of missing breaks.
> 
> Note that putting /* Fall through. */ before the use of the macro CASE_SEPARATORS does not work. Is it a (known) bug?

Yes, it's known that the "falls through" comments don't work if they're
preceding a macro.

	Marek

Re: Warning annoyances in list_read.c

[Dominique d'Humières]

> > If he added a new option affecting libgfortran, then he should
> > fix up libgfortran.
>
> He didn't add the warning to specifically annoy fortran developers.
> It is trivial to add seven gcc_fallthrough() or breaks for someone who
> knows the code and the person who added the warning obviously doesn't.
The following patch fixes the warnings

--- ../_clean/libgfortran/io/list_read.c	2017-03-25 20:42:40.000000000 +0100
+++ libgfortran/io/list_read.c	2017-03-27 12:06:10.000000000 +0200
@@ -51,7 +51,8 @@ typedef unsigned char uchar;
 #define CASE_DIGITS   case '0': case '1': case '2': case '3': case '4': \
                       case '5': case '6': case '7': case '8': case '9'
 
-#define CASE_SEPARATORS case ' ': case ',': case '/': case '\n': \
+#define CASE_SEPARATORS /* Fall through. */ \
+	case ' ': case ',': case '/': case '\n': \
 	case '\t': case '\r': case ';'
 
 /* This macro assumes that we're operating on a variable.  */

Indeed before applying this patch, someone will have to check that the warnings do not occur because of missing breaks.

Note that putting /* Fall through. */ before the use of the macro CASE_SEPARATORS does not work. Is it a (known) bug?

Dominique