Re: Implement -Wimplicit-fallthrough (version 9)

[Eric Botcazou]

> I really really don't see why anyone would think that those '...' bring
> any additional information.  Since Rainer has changed this, I see zero
> point in changing it back.

Yet doing it revealed an oversight in the first patch...

> It wasn't overlooked, there was a bug that I've fixed already which caused
> missing warnings.

Nope, see this hunk in the first patch:

@@ -4271,6 +4271,8 @@ convert (tree type, tree expr)
  return expr;
}
 
+  /* fall through */
+
 case CONSTRUCTOR:
   /* If we are converting a CONSTRUCTOR to a mere type variant, or to
 another padding type around the same type, just make a new one in

It's plain wrong, it should have been a break instead, but this was overlooked 
by everyone because of the bunch of false positives.

-- 
Eric Botcazou

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Sat, Oct 08, 2016 at 07:04:41PM +0200, Eric Botcazou wrote:
> > testing completed successfully, so I've installed the patch with this
> > ChangeLog entry:
> > 
> > 2016-09-26  Rainer Orth  
> > 
> > gcc:
> > * config/i386/i386.c (ix86_print_operand)
> > [HAVE_AS_IX86_CMOV_SUN_SYNTAX]: Add gcc_fallthrough.
> > * config/sparc/sparc.c (check_pic): Add fallthrough comment.
> > (epilogue_renumber): Likewise.
> > 
> > gcc/ada:
> > * gcc-interface/decl.c: Fix fall through comment formatting.
> > * gcc-interface/misc.c: Likewise.
> > * gcc-interface/trans.c: Likewise.
> > * gcc-interface/utils.c: Likewise.
> > * gcc-interface/utils2.c: Likewise.
> 
> This is a revealing example of how excessive pickiness in warnings can be 
> counter-productive: after Jakub's latest patches (thanks!) accepting the 
> original formatting of gcc-interface, I reverted part #2 of the above 
> patch... 
> only to discover that bootstrap was still broken because of a -Wimplicit-
> fallthrough warning, but this time for a missing break:

I really really don't see why anyone would think that those '...' bring
any additional information.  Since Rainer has changed this, I see zero
point in changing it back.

> Index: gcc-interface/utils.c
> ===
> --- gcc-interface/utils.c (revision 324591)
> +++ gcc-interface/utils.c (working copy)
> @@ -4289,6 +4289,7 @@ convert (tree type, tree expr)
> TREE_TYPE (expr) = type;
> return expr;
>   }
> +  break;
>  
>  case CONSTRUCTOR:
>/* If we are converting a CONSTRUCTOR to a mere type variant, or to
> 
> So the issue went unnoticed among the slew of false positives the first time 
> and a genuine error was overlooked...

It wasn't overlooked, there was a bug that I've fixed already which caused
missing warnings.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Eric Botcazou]

> testing completed successfully, so I've installed the patch with this
> ChangeLog entry:
> 
> 2016-09-26  Rainer Orth  
> 
>   gcc:
>   * config/i386/i386.c (ix86_print_operand)
>   [HAVE_AS_IX86_CMOV_SUN_SYNTAX]: Add gcc_fallthrough.
>   * config/sparc/sparc.c (check_pic): Add fallthrough comment.
>   (epilogue_renumber): Likewise.
> 
>   gcc/ada:
>   * gcc-interface/decl.c: Fix fall through comment formatting.
>   * gcc-interface/misc.c: Likewise.
>   * gcc-interface/trans.c: Likewise.
>   * gcc-interface/utils.c: Likewise.
>   * gcc-interface/utils2.c: Likewise.

This is a revealing example of how excessive pickiness in warnings can be 
counter-productive: after Jakub's latest patches (thanks!) accepting the 
original formatting of gcc-interface, I reverted part #2 of the above patch... 
only to discover that bootstrap was still broken because of a -Wimplicit-
fallthrough warning, but this time for a missing break:

Index: gcc-interface/utils.c
===
--- gcc-interface/utils.c   (revision 324591)
+++ gcc-interface/utils.c   (working copy)
@@ -4289,6 +4289,7 @@ convert (tree type, tree expr)
  TREE_TYPE (expr) = type;
  return expr;
}
+  break;
 
 case CONSTRUCTOR:
   /* If we are converting a CONSTRUCTOR to a mere type variant, or to

So the issue went unnoticed among the slew of false positives the first time 
and a genuine error was overlooked...

Tested on x86_64-suse-linux, applied on the mainline.


2016-10-08  Eric Botcazou  

* gcc-interface/utils.c (convert) : Add missing break.

Revert
2016-09-26  Rainer Orth  

* gcc-interface/decl.c: Fix fall through comment formatting.
* gcc-interface/misc.c: Likewise.
* gcc-interface/trans.c: Likewise.
* gcc-interface/utils.c: Likewise.
* gcc-interface/utils2.c: Likewise.


-- 
Eric BotcazouIndex: gcc-interface/decl.c
===
--- gcc-interface/decl.c	(revision 240888)
+++ gcc-interface/decl.c	(working copy)
@@ -596,7 +596,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit
 	gnu_expr
 	  = gnat_to_gnu_external (Expression (Declaration_Node (gnat_entity)));
 
-  /* fall through */
+  /* ... fall through ... */
 
 case E_Exception:
 case E_Loop_Parameter:
@@ -3369,7 +3369,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit
 	  break;
 	}
 
-  /* fall through */
+  /* ... fall through ... */
 
 case E_Record_Subtype:
   /* If Cloned_Subtype is Present it means this record subtype has
@@ -3804,7 +3804,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit
 	break;
 	}
 
-  /* fall through */
+  /* ... fall through ... */
 
 case E_Allocator_Type:
 case E_Access_Type:
@@ -6882,7 +6882,7 @@ choices_to_gnu (tree operand, Node_Id ch
 	  break;
 	}
 
-	  /* fall through */
+	  /* ... fall through ... */
 
 	case N_Character_Literal:
 	case N_Integer_Literal:
@@ -8089,7 +8089,7 @@ annotate_value (tree gnu_size)
   else
 	return Uint_Minus_1;
 
-  /* fall through */
+  /* Fall through... */
 
 default:
   return No_Uint;
Index: gcc-interface/misc.c
===
--- gcc-interface/misc.c	(revision 240888)
+++ gcc-interface/misc.c	(working copy)
@@ -157,7 +157,7 @@ gnat_handle_option (size_t scode, const
 case OPT_gant:
   warning (0, "%<-gnat%> misspelled as %<-gant%>");
 
-  /* fall through */
+  /* ... fall through ... */
 
 case OPT_gnat:
 case OPT_gnatO:
@@ -485,13 +485,13 @@ gnat_print_type (FILE *file, tree node,
   else
 	print_node (file, "index type", TYPE_INDEX_TYPE (node), indent + 4);
 
-  /* fall through */
+  /* ... fall through ... */
 
 case ENUMERAL_TYPE:
 case BOOLEAN_TYPE:
   print_node_brief (file, "RM size", TYPE_RM_SIZE (node), indent + 4);
 
-  /* fall through */
+  /* ... fall through ... */
 
 case REAL_TYPE:
   print_node_brief (file, "RM min", TYPE_RM_MIN_VALUE (node), indent + 4);
Index: gcc-interface/trans.c
===
--- gcc-interface/trans.c	(revision 240888)
+++ gcc-interface/trans.c	(working copy)
@@ -844,7 +844,7 @@ lvalue_required_p (Node_Id gnat_node, tr
 		 && Ekind (Entity (gnat_temp)) == E_Enumeration_Literal))
 	  return 1;
 
-  /* fall through */
+  /* ... fall through ... */
 
 case N_Slice:
   /* Only the array expression can require an lvalue.  */
@@ -890,7 +890,7 @@ lvalue_required_p (Node_Id gnat_node, tr
 	if (!constant)
 	  return 1;
 
-  /* fall through */
+  /* ... fall through ... */
 
 case N_Type_Conversion:
 case N_Qualified_Expression:
@@ -914,7 +914,7 @@ lvalue_required_p (Node_Id gnat_node, tr
 			

Re: Implement -Wimplicit-fallthrough (version 9)

[Tom Tromey]

> "Jakub" == Jakub Jelinek  writes:

>> default:
>> {
>> complaint (_complaints,
>> _("Storage class %d not recognized during scan"),
>> sclass);
>> }
>> /* FALLTHROUGH */
>> 
>> /* C_FCN is .bf and .ef symbols.  I think it is sufficient
>> to handle only the C_FUN and C_EXT.  */
>> case C_FCN:

Jakub> Is complaint a noreturn call?

Nope.

Jakub> but right now we only look for such comments immediately before a
Jakub> case/default keyword or user label; if there is another comment
Jakub> in between, it is ignored.  This is something we are considering
Jakub> to change, exactly because often the /* FALLTHRU */ comment
Jakub> appears after some case and then there is unrelated comment
Jakub> before the next case about what that case handles.

Make sense.  Thanks.

Tom

Re: Implement -Wimplicit-fallthrough (version 9)

[Bernd Schmidt]

On 09/28/2016 02:15 PM, Michael Matz wrote:

P.S.: Initially I even wanted to argue that the mere existence of _any_
comment before a case label would disable the warning.  I don't have the
numbers but I bet even that version would have found the very same bugs
that the picky version has.


Sounds like a pretty good idea to me for a default setting. If we really 
want to have multiple levels of the warning. I agree that it's likely to 
find the majority of problems, and it no longer depends on language and 
spelling of the comment.



Bernd

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Wed, Sep 28, 2016 at 09:29:01AM -0600, Tom Tromey wrote:
> > "Michael" == Michael Matz  writes:
> 
> Michael> Not accepting
> Michael>   /* And here we intentionally fall through because ... */
> Michael> and forcing users to replace this by:
> Michael>   /* fallthrough */
> Michael> is not robust either.  It's actually actively lowering robustness of 
> code, 
> Michael> it creates work for programmers that will be regarded as pointless 
> (and 
> Michael> rightly so) and will merely lead to everybody disabling the warning 
> (see 
> Michael> our generated files)
> 
> We can't control what programmers might do.  My point is that accepting
> too much is actively bad -- it hides errors.  If this somehow makes some
> programmer fall down a slippery slope, well, that's their error, not
> gcc's.
> 
> TBH I think it would be better not to parse comments at all.  Heuristics
> are generally bad and this case and ensuing discussion is a great
> demonstration of that.
> 
> The other day I built gdb with -Wimplicit-fallthrough.  I was surprised
> to find that gcc rejected this:
> 
>   default:
> {
>   complaint (_complaints,
>  _("Storage class %d not recognized during scan"),
>  sclass);
> }
> /* FALLTHROUGH */
> 
> /* C_FCN is .bf and .ef symbols.  I think it is sufficient
>to handle only the C_FUN and C_EXT.  */
>   case C_FCN:
> 
> Presumably without the comment heuristic, this would be accepted.

Is complaint a noreturn call?  If not, then it would certainly warn, unless
there is [[fallthrough]]; or __attribute__((fallthrough)); etc. (or the
comment).  For the comment, /* FALLTHROUGH */ is the recognized spelling of
the comment, but right now we only look for such comments immediately before
a case/default keyword or user label; if there is another comment in
between, it is ignored.  This is something we are considering to change,
exactly because often the /* FALLTHRU */ comment appears after some case and
then there is unrelated comment before the next case about what that case
handles.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Tom Tromey]

> "Michael" == Michael Matz  writes:

Michael> Not accepting
Michael>   /* And here we intentionally fall through because ... */
Michael> and forcing users to replace this by:
Michael>   /* fallthrough */
Michael> is not robust either.  It's actually actively lowering robustness of 
code, 
Michael> it creates work for programmers that will be regarded as pointless 
(and 
Michael> rightly so) and will merely lead to everybody disabling the warning 
(see 
Michael> our generated files)

We can't control what programmers might do.  My point is that accepting
too much is actively bad -- it hides errors.  If this somehow makes some
programmer fall down a slippery slope, well, that's their error, not
gcc's.

TBH I think it would be better not to parse comments at all.  Heuristics
are generally bad and this case and ensuing discussion is a great
demonstration of that.

The other day I built gdb with -Wimplicit-fallthrough.  I was surprised
to find that gcc rejected this:

default:
  {
complaint (_complaints,
   _("Storage class %d not recognized during scan"),
   sclass);
  }
  /* FALLTHROUGH */

  /* C_FCN is .bf and .ef symbols.  I think it is sufficient
 to handle only the C_FUN and C_EXT.  */
case C_FCN:

Presumably without the comment heuristic, this would be accepted.

Michael> The point of warnings is to make code robust under the condition of 
not 
Michael> being a pain by giving zillions of false positives.

My experience so far is that it's not so bad.  gdb actually had comments
in most spots, they just required a quick pass to clean them up:

https://sourceware.org/ml/gdb-patches/2016-09/msg00375.html

And, code bases in more dire straights can just disable the warning after all.

Tom

Re: Implement -Wimplicit-fallthrough (version 9)

[Michael Matz]

Hi,

On Tue, 27 Sep 2016, Tom Tromey wrote:

> The point of the warning is to make code more robust.  But accepting any 
> comment like "Don't fall through" is not more robust, but rather an 
> error waiting to happen; as IIUC the user has no way to detect this 
> case.
> 
> I think it's better for the comment-scanning feature to be very picky 
> (or even just not exist at all) -- that way you know exactly what you 
> are getting.  Lint was traditionally picky IIRC.  And, this is a warning 
> that isn't default and can also be disabled, so it's not as if users 
> have no recourse.

Not accepting
  /* And here we intentionally fall through because ... */
and forcing users to replace this by:
  /* fallthrough */
is not robust either.  It's actually actively lowering robustness of code, 
it creates work for programmers that will be regarded as pointless (and 
rightly so) and will merely lead to everybody disabling the warning (see 
our generated files) at which point we could just as well not have 
implemented it (which would be a shame because I think it's genuinely 
useful).

The point of warnings is to make code robust under the condition of not 
being a pain by giving zillions of false positives.  In this specific case 
the chance of giving false positives by being picky in how to disable the 
warning is very high.  On the other hand the chance of unintentionally 
disabling the warning by a negative comment like "Don't fall through here" 
is low, because presumably the one adding that comment (and hence thinking 
about that part of the code) also in fact put in the "break;" afterwards.

The argument with lint being picky would apply only if GCC would have 
added this warning maybe 20 years ago, not now where nearly nobody even 
knows what lint is, which lead to a large existing code base not having 
comments that would be accepted by lint but comments that do specify the 
intent of falling through.


Ciao,
Michael.
P.S.: Initially I even wanted to argue that the mere existence of _any_ 
comment before a case label would disable the warning.  I don't have the 
numbers but I bet even that version would have found the very same bugs 
that the picky version has.

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 01:31:15PM +0200, Jakub Jelinek wrote:
> On Tue, Sep 27, 2016 at 12:56:29PM +0200, Marek Polacek wrote:
> > On Tue, Sep 27, 2016 at 12:47:50PM +0200, Jakub Jelinek wrote:
> > > On Tue, Sep 27, 2016 at 12:39:41PM +0200, Markus Trippelsdorf wrote:
> > > > On 2016.09.27 at 10:46 +0200, Eric Botcazou wrote:
> > > > > > The intent has been that we catch the most common forms, but still 
> > > > > > require
> > > > > > it not to be complete free form.  Because, as experience shows, 
> > > > > > people are
> > > > > > extremely creative in these comments, and it is not very good idea 
> > > > > > to
> > > > > > support everything.  For ... fall through ... , what is the purpose 
> > > > > > of
> > > > > > those ...s?
> > > > > 
> > > > > No idea, but it has been there for a while and seems perfectly 
> > > > > reasonable.
> > > > > IMO any sentence containing "fall" and "through/thru/etc" on the same 
> > > > > line 
> > > > > should be accepted, otherwise it's just misplaced pickiness.
> > > > 
> > > > +1. Folks will just disable the warning if gcc is not very permissive
> > > > when paring existing comments. You cannot expect anyone to change
> > > > perfectly fine fall-through comments just to accommodate an arbitrary
> > > > gcc style.
> > > 
> > > The accepted style is already very permissive, we don't allow just one
> > > spelling as various lint tools.  I'm afraid looking for various cases of
> > > fall and through/thru possibly separated by anything and surrounded by
> > > anything is IMHO already too much, the compiler shouldn't try to try to
> > > grammar analyze the comments on what they actually talk about and whether 
> > > it
> > > might be related to the switch fall through or something completely
> > > different.  Users should start using [[fallthrough]]; anyway.
> > 
> > I'm thinking perhaps we should also accept /* ... fall through ... */
> > and /* else fall through */, but accepting any sentence containing "fall" 
> > and
> > "through/thru/etc" on the same line would mean that we also accept
> > /* Don't fall through here.  */ and that is clearly not desirable.
> 
> I think it is important to think in terms of what regexps we still want to
> match, even when the matching is actually implemented in C, not using
> regexps.  And yes, you list one reason why arbitrary text with fall and
> through somewhere in it is not a good idea.  Another:
> /* XXX Really fallthru?  */
> (what we have in pch.c).
> So, if you want to allow ... fall through ... and else fall through, and
> perhaps // fall through - some explanation
> then it might be e.g.
> //-fallthrough$
> //@fallthrough@$
> /\*-fallthrough\*/
> /\*@fallthrough@\*/
> //[ \t.]*(ELSE )?FALL(S | |-)?THR(OUGH|U)[ \t.]*(-.*)?$
> //[ \t.]*(Else )?Fall(s | |-)?[Tt]hr(ough|u)[ \t.]*(-.*)?$
> //[ \t.]*(else )?fall(s | |-)?thr(ough|u)[ \t.]*(-.*)?$
> /\*[ \t.]*(ELSE )?FALL(S | |-)?THR(OUGH|U)[ \t.]*(-.*)?\*/
> /\*[ \t.]*(Else )?Fall(s | |-)?[Tt]hr(ough|u)[ \t.]*(-.*)?\*/
> /\*[ \t.]*(else )?fall(s | |-)?thr(ough|u)[ \t.]*(-.*)?\*/
> where . would match even newlines in the last 3,
> but $ would always match just end of line?

This looks like a step in the right direction.  Apparently, it's hard
to come up with something that will make everyone happy; this might be
partly because GCC is probably the only compiler that attempts to
parse comments like this.  While clang has the implicit fallthrough
warning, they aren't even trying to parse the comments.

Jakub, do you want me to look into this (make GCC accept more), or do
you want to adjust that by yourself?

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Tom Tromey]

> "Michael" == Michael Matz  writes:

Michael> All those bugs would also have been found as well when it had simply 
Michael> accepted
Michael>   /fall.*thr/i
Michael> anywhere in the preceding comment on one line.  But all the recent 
Michael> spelling changes of comments to cater for the strictness exactly shows 
how 
Michael> misguided that is.  The above would accept "Don't fall through" as 
well.  
Michael> I say: so what?

The point of the warning is to make code more robust.  But accepting any
comment like "Don't fall through" is not more robust, but rather an
error waiting to happen; as IIUC the user has no way to detect this
case.

I think it's better for the comment-scanning feature to be very picky
(or even just not exist at all) -- that way you know exactly what you
are getting.  Lint was traditionally picky IIRC.  And, this is a warning
that isn't default and can also be disabled, so it's not as if users
have no recourse.

Tom

Re: Implement -Wimplicit-fallthrough (version 9)

[Michael Matz]

Hi,

On Tue, 27 Sep 2016, Marek Polacek wrote:

> > Perhaps we want -Wimplicit-fallthrough{,=1,=2,=3,=4}, where =1 would 
> > match indeed /fall.*thr/i (note, it will be really costly in this 
> > case, one will have to parse all comments in detail in the 
> > preprocessor, so I'd be against making it the default),
> 
> Perhaps we could use POSIX regcomp/regex functions; do you (or anyone 
> else) have an idea how expensive they are and if it's feasible to use 
> them in the preprocessor?

Why?  The regexp I gave was for demonstration.  Matching /fall.*thr/i 
would be done by something similar to:

  a = strcasestr(comment, "fall");
  if (!a) return;  // no fall
  b = strcasestr(a+4, "thr");
  if (!b) return;  // no thr
  if (memchr(a+4, '\n', b-a-4)) return; // on different lines
  foundit();

(With appropriate massaging that the comment to parse ends with 0.  
strcasestr would need addition to libiberty for where it's not available 
(or falling back to strstr there); obviously the above can be sped up by 
various tricks for ASCII and UTF-8 because of the relation of upper and 
lower case characters.  During tokenizing the comment (i.e. while 
searching for the end) one could already search for "fall" for instance to 
quickly early-out.)


Ciao,
Michael.

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 05:19:10PM +0200, Bernd Schmidt wrote:
> On 09/27/2016 05:04 PM, Jakub Jelinek wrote:
> >>>On Tue, Sep 27, 2016 at 9:56 AM, Michael Matz  wrote:
> All those bugs would also have been found as well when it had simply
> accepted
>   /fall.*thr/i
> anywhere in the preceding comment on one line.  But all the recent
> spelling changes of comments to cater for the strictness exactly shows how
> misguided that is.  The above would accept "Don't fall through" as well.
> I say: so what?
> 
> >Perhaps we want -Wimplicit-fallthrough{,=1,=2,=3,=4}, where
> >=1 would match indeed /fall.*thr/i (note, it will be really costly in this
> >case, one will have to parse all comments in detail in the preprocessor,
> >so I'd be against making it the default), =2 would allow
> >what we do right now, perhaps with the optional else and dots (perhaps
> >selected other interpunction chars), =3 would only allow the standardized
> >lint comments and =4 would not allow any comments, just the attributes?
> >Then each project can choose what they want.
> 
> I feel that's overthinking it. I believe Michael has identified the correct
> way to think about the issue.

See above, it is very expensive at preprocessing time (look at how the
preprocessor optimizes skipping over comments, with that it is all gone),
and not everybody will want /* Don't fall through here.  */ or
/* This is fallible.  Threats are high.  */ (pick any of the hundreds+
english words with fall in them and thousands+ of words with thr in them)
to disable the warning.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 05:04:23PM +0200, Jakub Jelinek wrote:
> On Tue, Sep 27, 2016 at 04:54:28PM +0200, Marek Polacek wrote:
> > On Tue, Sep 27, 2016 at 10:48:50AM -0400, Jason Merrill wrote:
> > > On Tue, Sep 27, 2016 at 9:56 AM, Michael Matz  wrote:
> > > > On Tue, 27 Sep 2016, Jakub Jelinek wrote:
> > > >
> > > >> Just compare that to the number of real bugs the warning found in gcc
> > > >> codebase.  It is really worth it for -Wextra.
> > > >
> > > > All those bugs would also have been found as well when it had simply
> > > > accepted
> > > >   /fall.*thr/i
> > > > anywhere in the preceding comment on one line.  But all the recent
> > > > spelling changes of comments to cater for the strictness exactly shows 
> > > > how
> > > > misguided that is.  The above would accept "Don't fall through" as well.
> > > > I say: so what?
> > > 
> > > I agree.
> > 
> > All right, I'm not opposed to making the comment parsing more benevolent.
> > We still should have enough time to fine-tune it.
> 
> Perhaps we want -Wimplicit-fallthrough{,=1,=2,=3,=4}, where
> =1 would match indeed /fall.*thr/i (note, it will be really costly in this
> case, one will have to parse all comments in detail in the preprocessor,
> so I'd be against making it the default),

Perhaps we could use POSIX regcomp/regex functions; do you (or anyone else)
have an idea how expensive they are and if it's feasible to use them in the
preprocessor?

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Bernd Schmidt]

On 09/27/2016 05:04 PM, Jakub Jelinek wrote:

On Tue, Sep 27, 2016 at 9:56 AM, Michael Matz  wrote:

All those bugs would also have been found as well when it had simply
accepted
  /fall.*thr/i
anywhere in the preceding comment on one line.  But all the recent
spelling changes of comments to cater for the strictness exactly shows how
misguided that is.  The above would accept "Don't fall through" as well.
I say: so what?



Perhaps we want -Wimplicit-fallthrough{,=1,=2,=3,=4}, where
=1 would match indeed /fall.*thr/i (note, it will be really costly in this
case, one will have to parse all comments in detail in the preprocessor,
so I'd be against making it the default), =2 would allow
what we do right now, perhaps with the optional else and dots (perhaps
selected other interpunction chars), =3 would only allow the standardized
lint comments and =4 would not allow any comments, just the attributes?
Then each project can choose what they want.


I feel that's overthinking it. I believe Michael has identified the 
correct way to think about the issue.



Bernd

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 04:54:28PM +0200, Marek Polacek wrote:
> On Tue, Sep 27, 2016 at 10:48:50AM -0400, Jason Merrill wrote:
> > On Tue, Sep 27, 2016 at 9:56 AM, Michael Matz  wrote:
> > > On Tue, 27 Sep 2016, Jakub Jelinek wrote:
> > >
> > >> Just compare that to the number of real bugs the warning found in gcc
> > >> codebase.  It is really worth it for -Wextra.
> > >
> > > All those bugs would also have been found as well when it had simply
> > > accepted
> > >   /fall.*thr/i
> > > anywhere in the preceding comment on one line.  But all the recent
> > > spelling changes of comments to cater for the strictness exactly shows how
> > > misguided that is.  The above would accept "Don't fall through" as well.
> > > I say: so what?
> > 
> > I agree.
> 
> All right, I'm not opposed to making the comment parsing more benevolent.
> We still should have enough time to fine-tune it.

Perhaps we want -Wimplicit-fallthrough{,=1,=2,=3,=4}, where
=1 would match indeed /fall.*thr/i (note, it will be really costly in this
case, one will have to parse all comments in detail in the preprocessor,
so I'd be against making it the default), =2 would allow
what we do right now, perhaps with the optional else and dots (perhaps
selected other interpunction chars), =3 would only allow the standardized
lint comments and =4 would not allow any comments, just the attributes?
Then each project can choose what they want.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 10:48:50AM -0400, Jason Merrill wrote:
> On Tue, Sep 27, 2016 at 9:56 AM, Michael Matz  wrote:
> > On Tue, 27 Sep 2016, Jakub Jelinek wrote:
> >
> >> Just compare that to the number of real bugs the warning found in gcc
> >> codebase.  It is really worth it for -Wextra.
> >
> > All those bugs would also have been found as well when it had simply
> > accepted
> >   /fall.*thr/i
> > anywhere in the preceding comment on one line.  But all the recent
> > spelling changes of comments to cater for the strictness exactly shows how
> > misguided that is.  The above would accept "Don't fall through" as well.
> > I say: so what?
> 
> I agree.

All right, I'm not opposed to making the comment parsing more benevolent.
We still should have enough time to fine-tune it.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Jason Merrill]

On Tue, Sep 27, 2016 at 9:56 AM, Michael Matz  wrote:
> On Tue, 27 Sep 2016, Jakub Jelinek wrote:
>
>> Just compare that to the number of real bugs the warning found in gcc
>> codebase.  It is really worth it for -Wextra.
>
> All those bugs would also have been found as well when it had simply
> accepted
>   /fall.*thr/i
> anywhere in the preceding comment on one line.  But all the recent
> spelling changes of comments to cater for the strictness exactly shows how
> misguided that is.  The above would accept "Don't fall through" as well.
> I say: so what?

I agree.

Jason

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 03:53:25PM +0200, Bernd Schmidt wrote:
> What's the ratio of comments "fixed" to actual bugs found? IMO this is not
> something we should inflict on users unasked.

One might argue that users actually *asked* for this by turning on -Wextra.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Bernd Schmidt]

On 09/27/2016 03:49 PM, Jakub Jelinek wrote:

On Tue, Sep 27, 2016 at 03:48:07PM +0200, Bernd Schmidt wrote:

On 09/27/2016 02:01 PM, Marek Polacek wrote:

On Tue, Sep 27, 2016 at 01:55:22PM +0200, Bernd Schmidt wrote:

On 09/27/2016 01:51 PM, Marek Polacek wrote:

But the C/C++ keywords are all English, too; lint tools only accept English,
and so it wouldn't seem unreasonable to only accept English keywords in the
comments.  And in any case, I don't see how a compiler can be expected to
be able to parse non-English languages.


It isn't. But it can also be reasonably by expected not to warn about things
that are valid according to the language specification and are frequently
used.


Ok, but note that the warning is in -Wextra, not enabled by default/-Wall.


I think it's problematic enough that it needs to be removed from -Wextra as
well. The latest ia64 backend patch shows that clearly IMO.


Just compare that to the number of real bugs the warning found in gcc
codebase.  It is really worth it for -Wextra.


What's the ratio of comments "fixed" to actual bugs found? IMO this is 
not something we should inflict on users unasked.



Bernd

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 03:53:25PM +0200, Bernd Schmidt wrote:
> On 09/27/2016 03:49 PM, Jakub Jelinek wrote:
> >On Tue, Sep 27, 2016 at 03:48:07PM +0200, Bernd Schmidt wrote:
> >>On 09/27/2016 02:01 PM, Marek Polacek wrote:
> >>>On Tue, Sep 27, 2016 at 01:55:22PM +0200, Bernd Schmidt wrote:
> On 09/27/2016 01:51 PM, Marek Polacek wrote:
> >But the C/C++ keywords are all English, too; lint tools only accept 
> >English,
> >and so it wouldn't seem unreasonable to only accept English keywords in 
> >the
> >comments.  And in any case, I don't see how a compiler can be expected to
> >be able to parse non-English languages.
> 
> It isn't. But it can also be reasonably by expected not to warn about 
> things
> that are valid according to the language specification and are frequently
> used.
> >>>
> >>>Ok, but note that the warning is in -Wextra, not enabled by default/-Wall.
> >>
> >>I think it's problematic enough that it needs to be removed from -Wextra as
> >>well. The latest ia64 backend patch shows that clearly IMO.
> >
> >Just compare that to the number of real bugs the warning found in gcc
> >codebase.  It is really worth it for -Wextra.
> 
> What's the ratio of comments "fixed" to actual bugs found? IMO this is not
> something we should inflict on users unasked.

We've inflicted on users many other coding style warnings that have a
reasonably high chance to reveal real bugs, e.g. -Wmisleading-indentation which 
is
even enabled in -Wall, not just -Wextra.  In reality, -Wextra isn't used
that often, and the people that use it will most likely benefit from the
warning IMNSHO.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Michael Matz]

Hi,

On Tue, 27 Sep 2016, Jakub Jelinek wrote:

> Just compare that to the number of real bugs the warning found in gcc 
> codebase.  It is really worth it for -Wextra.

All those bugs would also have been found as well when it had simply 
accepted
  /fall.*thr/i
anywhere in the preceding comment on one line.  But all the recent 
spelling changes of comments to cater for the strictness exactly shows how 
misguided that is.  The above would accept "Don't fall through" as well.  
I say: so what?


Ciao,
Michael.

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 03:48:07PM +0200, Bernd Schmidt wrote:
> On 09/27/2016 02:01 PM, Marek Polacek wrote:
> >On Tue, Sep 27, 2016 at 01:55:22PM +0200, Bernd Schmidt wrote:
> >>On 09/27/2016 01:51 PM, Marek Polacek wrote:
> >>>But the C/C++ keywords are all English, too; lint tools only accept 
> >>>English,
> >>>and so it wouldn't seem unreasonable to only accept English keywords in the
> >>>comments.  And in any case, I don't see how a compiler can be expected to
> >>>be able to parse non-English languages.
> >>
> >>It isn't. But it can also be reasonably by expected not to warn about things
> >>that are valid according to the language specification and are frequently
> >>used.
> >
> >Ok, but note that the warning is in -Wextra, not enabled by default/-Wall.
> 
> I think it's problematic enough that it needs to be removed from -Wextra as
> well. The latest ia64 backend patch shows that clearly IMO.

Just compare that to the number of real bugs the warning found in gcc
codebase.  It is really worth it for -Wextra.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Bernd Schmidt]

On 09/27/2016 02:01 PM, Marek Polacek wrote:

On Tue, Sep 27, 2016 at 01:55:22PM +0200, Bernd Schmidt wrote:

On 09/27/2016 01:51 PM, Marek Polacek wrote:

But the C/C++ keywords are all English, too; lint tools only accept English,
and so it wouldn't seem unreasonable to only accept English keywords in the
comments.  And in any case, I don't see how a compiler can be expected to
be able to parse non-English languages.


It isn't. But it can also be reasonably by expected not to warn about things
that are valid according to the language specification and are frequently
used.


Ok, but note that the warning is in -Wextra, not enabled by default/-Wall.


I think it's problematic enough that it needs to be removed from -Wextra 
as well. The latest ia64 backend patch shows that clearly IMO.



Bernd

Re: Implement -Wimplicit-fallthrough (version 9)

[Segher Boessenkool]

On Tue, Sep 27, 2016 at 01:58:54PM +0200, Jakub Jelinek wrote:
> > Any comment with text
> > 
> > ^[^_[:alnum:]]*(else )?fall(s | |-)?thr(ough|u)[^_[:alnum:]]*$
> > 
> > perhaps?  Case-insensitive.  Or allow any amount of space, or even any
> > interpunction.  Just don't allow any alphanumerics except for those
> > exact words, and there won't be many false hits at all.
> 
> Not sure we want to match FaLlS THrouGH,

Yes it's silly, but would it ever match the wrong thing?

> and [^_[:alnum:]]* isn't without a
> problem either, what if there is hebrew, or chinese, etc. text in there?

I meant in LANG=C, but it would work otherwise, too.  Nasty, of course.

> The matching shouldn't depend on the current locale IMHO, and figuring out 
> what
> unicode entry points are letters and which are not really isn't easy without 
> that.

Right.

> IMO before changing anything further, we want to gather some statistics what
> styles are actually used in the wild together with how often they are used,
> and then for the more common ones decide what is really supportable.

If you do not allow a lot then there will be many false negatives.


Segher

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 02:27:12PM +0200, Florian Weimer wrote:
> * Marek Polacek:
> 
> > On Tue, Sep 27, 2016 at 01:46:08PM +0200, Markus Trippelsdorf wrote:
> >> I'm also wondering about the situation where not a single break is used
> >> in all of the cases. It would be best not to warn here.
> >
> > This is tricky and I'm afraid all I can offer here is to use the diagnostics
> > pragma to suppress the warning for Duff's device-like constructs.
> 
> Would it make sense to apply the fallthrough attribute to the entire
> switch statement to address such scenarios?  Currently, that does not
> seem supported.

I've been thinking about this, too.  But I think we'd have to invent
a new attribute, e.g. no_warn_fallthrough or so.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 02:27:12PM +0200, Florian Weimer wrote:
> * Marek Polacek:
> 
> > On Tue, Sep 27, 2016 at 01:46:08PM +0200, Markus Trippelsdorf wrote:
> >> I'm also wondering about the situation where not a single break is used
> >> in all of the cases. It would be best not to warn here.
> >
> > This is tricky and I'm afraid all I can offer here is to use the diagnostics
> > pragma to suppress the warning for Duff's device-like constructs.
> 
> Would it make sense to apply the fallthrough attribute to the entire
> switch statement to address such scenarios?  Currently, that does not
> seem supported.

Where the attribute is allowed or not allowed is currently intentionally
derived from where C++17 allows it.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Florian Weimer]

* Marek Polacek:

> On Tue, Sep 27, 2016 at 01:46:08PM +0200, Markus Trippelsdorf wrote:
>> I'm also wondering about the situation where not a single break is used
>> in all of the cases. It would be best not to warn here.
>
> This is tricky and I'm afraid all I can offer here is to use the diagnostics
> pragma to suppress the warning for Duff's device-like constructs.

Would it make sense to apply the fallthrough attribute to the entire
switch statement to address such scenarios?  Currently, that does not
seem supported.

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 12:12:30PM +0100, Kyrill Tkachov wrote:
> 
> On 27/09/16 11:41, Jakub Jelinek wrote:
> > On Tue, Sep 27, 2016 at 11:32:42AM +0100, Kyrill Tkachov wrote:
> > > where the code is:
> > > 2156   /* Fall through - if the lane index isn't a constant 
> > > then
> > > 2157  the next case will error.  */
> > > 2158
> > > 2159 case NEON_ARG_CONSTANT:
> > > 
> > > 
> > > Is there supposed to be no empty line between the case statement and the 
> > > comment?
> > > Or is the comment only supposed to contain "Fall through"?
> > The last comment before case or default keyword (or user label before
> > case/default) has to match one of the following regexps:
> > //-fallthrough$
> > //@fallthrough@$
> > //[ \t]*FALL(S | |-)?THR(OUGH|U)\.?[ \t]*$
> > //[ \t]*Fall(s | |-)?[Tt]hr(ough|u)\.?[ \t]*$
> > //[ \t]*fall(s | |-)?thr(ough|u)\.?[ \t]*$
> > /\*-fallthrough\*/
> > /\*@fallthrough@\*/
> > /\*[ \t]*FALL(S | |-)?THR(OUGH|U)\.?[ \t]*\*/
> > /\*[ \t]*Fall(s | |-)?[Tt]hr(ough|u)\.?[ \t]*\*/
> > /\*[ \t]*fall(s | |-)?thr(ough|u)\.?[ \t]*\*/
> > 
> > So, you could e.g. write:
> > /* If the lane index isn't a constant, then the next case will error.  
> > */
> > /* Fall through.  */
> > but not what you have, free form is not accepted.
> Thanks. Given the discussion going on about the acceptable comment formats,
> is it preferable to use comments in the gcc codebase at all, or should I
> use gcc_fallthrough () (with an explanatory comment if needed)?

It's probably that the comments are preferable, but sometimes you can't use
them (if e.g. something like CASE_CONVERT or another comment or } follows).

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 01:46:08PM +0200, Markus Trippelsdorf wrote:
> I'm also wondering about the situation where not a single break is used
> in all of the cases. It would be best not to warn here.

This is tricky and I'm afraid all I can offer here is to use the diagnostics
pragma to suppress the warning for Duff's device-like constructs.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 01:55:22PM +0200, Bernd Schmidt wrote:
> On 09/27/2016 01:51 PM, Marek Polacek wrote:
> > But the C/C++ keywords are all English, too; lint tools only accept English,
> > and so it wouldn't seem unreasonable to only accept English keywords in the
> > comments.  And in any case, I don't see how a compiler can be expected to
> > be able to parse non-English languages.
> 
> It isn't. But it can also be reasonably by expected not to warn about things
> that are valid according to the language specification and are frequently
> used.

Ok, but note that the warning is in -Wextra, not enabled by default/-Wall.
I'm all for reducing false positives whenever possible and we can improve
out comment-parsing heuristics, but I just can't see us handling anything
other than English.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 06:51:31AM -0500, Segher Boessenkool wrote:
> On Tue, Sep 27, 2016 at 01:31:15PM +0200, Jakub Jelinek wrote:
> > I think it is important to think in terms of what regexps we still want to
> > match, even when the matching is actually implemented in C, not using
> > regexps.  And yes, you list one reason why arbitrary text with fall and
> > through somewhere in it is not a good idea.  Another:
> > /* XXX Really fallthru?  */
> > (what we have in pch.c).
> > So, if you want to allow ... fall through ... and else fall through, and
> > perhaps // fall through - some explanation
> > then it might be e.g.
> > //-fallthrough$
> > //@fallthrough@$
> > /\*-fallthrough\*/
> > /\*@fallthrough@\*/
> > //[ \t.]*(ELSE )?FALL(S | |-)?THR(OUGH|U)[ \t.]*(-.*)?$
> > //[ \t.]*(Else )?Fall(s | |-)?[Tt]hr(ough|u)[ \t.]*(-.*)?$
> > //[ \t.]*(else )?fall(s | |-)?thr(ough|u)[ \t.]*(-.*)?$
> > /\*[ \t.]*(ELSE )?FALL(S | |-)?THR(OUGH|U)[ \t.]*(-.*)?\*/
> > /\*[ \t.]*(Else )?Fall(s | |-)?[Tt]hr(ough|u)[ \t.]*(-.*)?\*/
> > /\*[ \t.]*(else )?fall(s | |-)?thr(ough|u)[ \t.]*(-.*)?\*/
> > where . would match even newlines in the last 3,
> > but $ would always match just end of line?
> 
> Any comment with text
> 
> ^[^_[:alnum:]]*(else )?fall(s | |-)?thr(ough|u)[^_[:alnum:]]*$
> 
> perhaps?  Case-insensitive.  Or allow any amount of space, or even any
> interpunction.  Just don't allow any alphanumerics except for those
> exact words, and there won't be many false hits at all.

Not sure we want to match FaLlS THrouGH, and [^_[:alnum:]]* isn't without a
problem either, what if there is hebrew, or chinese, etc. text in there?
The matching shouldn't depend on the current locale IMHO, and figuring out what
unicode entry points are letters and which are not really isn't easy without 
that.

IMO before changing anything further, we want to gather some statistics what
styles are actually used in the wild together with how often they are used,
and then for the more common ones decide what is really supportable.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Bernd Schmidt]

On 09/27/2016 01:51 PM, Marek Polacek wrote:

But the C/C++ keywords are all English, too; lint tools only accept English,
and so it wouldn't seem unreasonable to only accept English keywords in the
comments.  And in any case, I don't see how a compiler can be expected to
be able to parse non-English languages.


It isn't. But it can also be reasonably by expected not to warn about 
things that are valid according to the language specification and are 
frequently used.



Bernd

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 01:47:07PM +0200, Bernd Schmidt wrote:
> On 09/27/2016 01:09 PM, Richard Biener wrote:
> > On Tue, Sep 27, 2016 at 1:06 PM, Eric Botcazou  
> > wrote:
> > > > The accepted style is already very permissive, we don't allow just one
> > > > spelling as various lint tools.
> > > 
> > > Well, it cannot even handle the variations of a single codebase, GCC 
> > > itself,
> > > so I'm afraid very permissive is not exactly the appropriate wording here.
> > > Anyway, we'll see whether Bugzilla is flooded or not in the upcoming days.
> > 
> > During discussion I already pointed out that people may use non-english
> > variants as well.  I've seen a lot of french variable/function names in my
> > academic life for example.
> 
> Yes, I pointed out the same thing a few weeks ago.

But the C/C++ keywords are all English, too; lint tools only accept English,
and so it wouldn't seem unreasonable to only accept English keywords in the
comments.  And in any case, I don't see how a compiler can be expected to
be able to parse non-English languages.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Segher Boessenkool]

On Tue, Sep 27, 2016 at 01:31:15PM +0200, Jakub Jelinek wrote:
> I think it is important to think in terms of what regexps we still want to
> match, even when the matching is actually implemented in C, not using
> regexps.  And yes, you list one reason why arbitrary text with fall and
> through somewhere in it is not a good idea.  Another:
> /* XXX Really fallthru?  */
> (what we have in pch.c).
> So, if you want to allow ... fall through ... and else fall through, and
> perhaps // fall through - some explanation
> then it might be e.g.
> //-fallthrough$
> //@fallthrough@$
> /\*-fallthrough\*/
> /\*@fallthrough@\*/
> //[ \t.]*(ELSE )?FALL(S | |-)?THR(OUGH|U)[ \t.]*(-.*)?$
> //[ \t.]*(Else )?Fall(s | |-)?[Tt]hr(ough|u)[ \t.]*(-.*)?$
> //[ \t.]*(else )?fall(s | |-)?thr(ough|u)[ \t.]*(-.*)?$
> /\*[ \t.]*(ELSE )?FALL(S | |-)?THR(OUGH|U)[ \t.]*(-.*)?\*/
> /\*[ \t.]*(Else )?Fall(s | |-)?[Tt]hr(ough|u)[ \t.]*(-.*)?\*/
> /\*[ \t.]*(else )?fall(s | |-)?thr(ough|u)[ \t.]*(-.*)?\*/
> where . would match even newlines in the last 3,
> but $ would always match just end of line?

Any comment with text

^[^_[:alnum:]]*(else )?fall(s | |-)?thr(ough|u)[^_[:alnum:]]*$

perhaps?  Case-insensitive.  Or allow any amount of space, or even any
interpunction.  Just don't allow any alphanumerics except for those
exact words, and there won't be many false hits at all.


Segher

Re: Implement -Wimplicit-fallthrough (version 9)

[Bernd Schmidt]

On 09/27/2016 01:09 PM, Richard Biener wrote:

On Tue, Sep 27, 2016 at 1:06 PM, Eric Botcazou  wrote:

The accepted style is already very permissive, we don't allow just one
spelling as various lint tools.


Well, it cannot even handle the variations of a single codebase, GCC itself,
so I'm afraid very permissive is not exactly the appropriate wording here.
Anyway, we'll see whether Bugzilla is flooded or not in the upcoming days.


During discussion I already pointed out that people may use non-english
variants as well.  I've seen a lot of french variable/function names in my
academic life for example.


Yes, I pointed out the same thing a few weeks ago.

The warning does seem to be useful and discover errors, but I worry 
about the large amount of false positives it produces.



Bernd

Re: Implement -Wimplicit-fallthrough (version 9)

[Markus Trippelsdorf]

On 2016.09.27 at 12:56 +0200, Marek Polacek wrote:
> On Tue, Sep 27, 2016 at 12:47:50PM +0200, Jakub Jelinek wrote:
> > On Tue, Sep 27, 2016 at 12:39:41PM +0200, Markus Trippelsdorf wrote:
> > > On 2016.09.27 at 10:46 +0200, Eric Botcazou wrote:
> > > > > The intent has been that we catch the most common forms, but still 
> > > > > require
> > > > > it not to be complete free form.  Because, as experience shows, 
> > > > > people are
> > > > > extremely creative in these comments, and it is not very good idea to
> > > > > support everything.  For ... fall through ... , what is the purpose of
> > > > > those ...s?
> > > >
> > > > No idea, but it has been there for a while and seems perfectly 
> > > > reasonable.
> > > > IMO any sentence containing "fall" and "through/thru/etc" on the same 
> > > > line
> > > > should be accepted, otherwise it's just misplaced pickiness.
> > >
> > > +1. Folks will just disable the warning if gcc is not very permissive
> > > when paring existing comments. You cannot expect anyone to change
> > > perfectly fine fall-through comments just to accommodate an arbitrary
> > > gcc style.
> >
> > The accepted style is already very permissive, we don't allow just one
> > spelling as various lint tools.  I'm afraid looking for various cases of
> > fall and through/thru possibly separated by anything and surrounded by
> > anything is IMHO already too much, the compiler shouldn't try to try to
> > grammar analyze the comments on what they actually talk about and whether it
> > might be related to the switch fall through or something completely
> > different.  Users should start using [[fallthrough]]; anyway.
>
> I'm thinking perhaps we should also accept /* ... fall through ... */
> and /* else fall through */, but accepting any sentence containing "fall" and
> "through/thru/etc" on the same line would mean that we also accept
> /* Don't fall through here.  */ and that is clearly not desirable.
>

I'm also wondering about the situation where not a single break is used
in all of the cases. It would be best not to warn here.

An example from ffmpeg:

#define LPC1(x) {   \
int c = coefs[(x)-1];   \
p0   += MUL(c, s);  \
s = smp[i-(x)+1];   \
p1   += MUL(c, s);  \
}

static av_always_inline void FUNC(lpc_encode_unrolled)(int32_t *res,
  const int32_t *smp, int len, int order,
  const int32_t *coefs, int shift, int big)
{
int i;
for (i = order; i < len; i += 2) {
int s  = smp[i-order];
sum_type p0 = 0, p1 = 0;
if (big) {
switch (order) {
case 32: LPC1(32)
case 31: LPC1(31)
case 30: LPC1(30)
case 29: LPC1(29)
case 28: LPC1(28)
case 27: LPC1(27)
case 26: LPC1(26)
case 25: LPC1(25)
case 24: LPC1(24)
case 23: LPC1(23)
case 22: LPC1(22)
case 21: LPC1(21)
case 20: LPC1(20)
case 19: LPC1(19)
case 18: LPC1(18)
case 17: LPC1(17)
case 16: LPC1(16)
case 15: LPC1(15)
case 14: LPC1(14)
case 13: LPC1(13)
case 12: LPC1(12)
case 11: LPC1(11)
case 10: LPC1(10)
case  9: LPC1( 9)
 LPC1( 8)
 LPC1( 7)
 LPC1( 6)
 LPC1( 5)
 LPC1( 4)
 LPC1( 3)
 LPC1( 2)
 LPC1( 1)
}
} else {
switch (order) {
case  8: LPC1( 8)
case  7: LPC1( 7)
case  6: LPC1( 6)
case  5: LPC1( 5)
case  4: LPC1( 4)
case  3: LPC1( 3)
case  2: LPC1( 2)
case  1: LPC1( 1)
}
}
res[i  ] = smp[i  ] - CLIP(p0 >> shift);
res[i+1] = smp[i+1] - CLIP(p1 >> shift);
}
}


--
Markus

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 12:56:29PM +0200, Marek Polacek wrote:
> On Tue, Sep 27, 2016 at 12:47:50PM +0200, Jakub Jelinek wrote:
> > On Tue, Sep 27, 2016 at 12:39:41PM +0200, Markus Trippelsdorf wrote:
> > > On 2016.09.27 at 10:46 +0200, Eric Botcazou wrote:
> > > > > The intent has been that we catch the most common forms, but still 
> > > > > require
> > > > > it not to be complete free form.  Because, as experience shows, 
> > > > > people are
> > > > > extremely creative in these comments, and it is not very good idea to
> > > > > support everything.  For ... fall through ... , what is the purpose of
> > > > > those ...s?
> > > > 
> > > > No idea, but it has been there for a while and seems perfectly 
> > > > reasonable.
> > > > IMO any sentence containing "fall" and "through/thru/etc" on the same 
> > > > line 
> > > > should be accepted, otherwise it's just misplaced pickiness.
> > > 
> > > +1. Folks will just disable the warning if gcc is not very permissive
> > > when paring existing comments. You cannot expect anyone to change
> > > perfectly fine fall-through comments just to accommodate an arbitrary
> > > gcc style.
> > 
> > The accepted style is already very permissive, we don't allow just one
> > spelling as various lint tools.  I'm afraid looking for various cases of
> > fall and through/thru possibly separated by anything and surrounded by
> > anything is IMHO already too much, the compiler shouldn't try to try to
> > grammar analyze the comments on what they actually talk about and whether it
> > might be related to the switch fall through or something completely
> > different.  Users should start using [[fallthrough]]; anyway.
> 
> I'm thinking perhaps we should also accept /* ... fall through ... */
> and /* else fall through */, but accepting any sentence containing "fall" and
> "through/thru/etc" on the same line would mean that we also accept
> /* Don't fall through here.  */ and that is clearly not desirable.

I think it is important to think in terms of what regexps we still want to
match, even when the matching is actually implemented in C, not using
regexps.  And yes, you list one reason why arbitrary text with fall and
through somewhere in it is not a good idea.  Another:
/* XXX Really fallthru?  */
(what we have in pch.c).
So, if you want to allow ... fall through ... and else fall through, and
perhaps // fall through - some explanation
then it might be e.g.
//-fallthrough$
//@fallthrough@$
/\*-fallthrough\*/
/\*@fallthrough@\*/
//[ \t.]*(ELSE )?FALL(S | |-)?THR(OUGH|U)[ \t.]*(-.*)?$
//[ \t.]*(Else )?Fall(s | |-)?[Tt]hr(ough|u)[ \t.]*(-.*)?$
//[ \t.]*(else )?fall(s | |-)?thr(ough|u)[ \t.]*(-.*)?$
/\*[ \t.]*(ELSE )?FALL(S | |-)?THR(OUGH|U)[ \t.]*(-.*)?\*/
/\*[ \t.]*(Else )?Fall(s | |-)?[Tt]hr(ough|u)[ \t.]*(-.*)?\*/
/\*[ \t.]*(else )?fall(s | |-)?thr(ough|u)[ \t.]*(-.*)?\*/
where . would match even newlines in the last 3,
but $ would always match just end of line?

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Kyrill Tkachov]

On 27/09/16 11:41, Jakub Jelinek wrote:

On Tue, Sep 27, 2016 at 11:32:42AM +0100, Kyrill Tkachov wrote:

where the code is:
2156   /* Fall through - if the lane index isn't a constant then
2157  the next case will error.  */
2158
2159 case NEON_ARG_CONSTANT:


Is there supposed to be no empty line between the case statement and the 
comment?
Or is the comment only supposed to contain "Fall through"?

The last comment before case or default keyword (or user label before
case/default) has to match one of the following regexps:
//-fallthrough$
//@fallthrough@$
//[ \t]*FALL(S | |-)?THR(OUGH|U)\.?[ \t]*$
//[ \t]*Fall(s | |-)?[Tt]hr(ough|u)\.?[ \t]*$
//[ \t]*fall(s | |-)?thr(ough|u)\.?[ \t]*$
/\*-fallthrough\*/
/\*@fallthrough@\*/
/\*[ \t]*FALL(S | |-)?THR(OUGH|U)\.?[ \t]*\*/
/\*[ \t]*Fall(s | |-)?[Tt]hr(ough|u)\.?[ \t]*\*/
/\*[ \t]*fall(s | |-)?thr(ough|u)\.?[ \t]*\*/

So, you could e.g. write:
/* If the lane index isn't a constant, then the next case will error.  
*/
/* Fall through.  */
but not what you have, free form is not accepted.

Thanks. Given the discussion going on about the acceptable comment formats,
is it preferable to use comments in the gcc codebase at all, or should I
use gcc_fallthrough () (with an explanatory comment if needed)?

Kyrill


Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Richard Biener]

On Tue, Sep 27, 2016 at 1:06 PM, Eric Botcazou  wrote:
>> The accepted style is already very permissive, we don't allow just one
>> spelling as various lint tools.
>
> Well, it cannot even handle the variations of a single codebase, GCC itself,
> so I'm afraid very permissive is not exactly the appropriate wording here.
> Anyway, we'll see whether Bugzilla is flooded or not in the upcoming days.

During discussion I already pointed out that people may use non-english
variants as well.  I've seen a lot of french variable/function names in my
academic life for example.

Richard.

> --
> Eric Botcazou

Re: Implement -Wimplicit-fallthrough (version 9)

[Eric Botcazou]

> The accepted style is already very permissive, we don't allow just one
> spelling as various lint tools.

Well, it cannot even handle the variations of a single codebase, GCC itself, 
so I'm afraid very permissive is not exactly the appropriate wording here.
Anyway, we'll see whether Bugzilla is flooded or not in the upcoming days.

-- 
Eric Botcazou

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 12:47:50PM +0200, Jakub Jelinek wrote:
> On Tue, Sep 27, 2016 at 12:39:41PM +0200, Markus Trippelsdorf wrote:
> > On 2016.09.27 at 10:46 +0200, Eric Botcazou wrote:
> > > > The intent has been that we catch the most common forms, but still 
> > > > require
> > > > it not to be complete free form.  Because, as experience shows, people 
> > > > are
> > > > extremely creative in these comments, and it is not very good idea to
> > > > support everything.  For ... fall through ... , what is the purpose of
> > > > those ...s?
> > > 
> > > No idea, but it has been there for a while and seems perfectly reasonable.
> > > IMO any sentence containing "fall" and "through/thru/etc" on the same 
> > > line 
> > > should be accepted, otherwise it's just misplaced pickiness.
> > 
> > +1. Folks will just disable the warning if gcc is not very permissive
> > when paring existing comments. You cannot expect anyone to change
> > perfectly fine fall-through comments just to accommodate an arbitrary
> > gcc style.
> 
> The accepted style is already very permissive, we don't allow just one
> spelling as various lint tools.  I'm afraid looking for various cases of
> fall and through/thru possibly separated by anything and surrounded by
> anything is IMHO already too much, the compiler shouldn't try to try to
> grammar analyze the comments on what they actually talk about and whether it
> might be related to the switch fall through or something completely
> different.  Users should start using [[fallthrough]]; anyway.

I'm thinking perhaps we should also accept /* ... fall through ... */
and /* else fall through */, but accepting any sentence containing "fall" and
"through/thru/etc" on the same line would mean that we also accept
/* Don't fall through here.  */ and that is clearly not desirable.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 12:47:50PM +0200, Jakub Jelinek wrote:
> On Tue, Sep 27, 2016 at 12:39:41PM +0200, Markus Trippelsdorf wrote:
> > On 2016.09.27 at 10:46 +0200, Eric Botcazou wrote:
> > > > The intent has been that we catch the most common forms, but still 
> > > > require
> > > > it not to be complete free form.  Because, as experience shows, people 
> > > > are
> > > > extremely creative in these comments, and it is not very good idea to
> > > > support everything.  For ... fall through ... , what is the purpose of
> > > > those ...s?
> > > 
> > > No idea, but it has been there for a while and seems perfectly reasonable.
> > > IMO any sentence containing "fall" and "through/thru/etc" on the same 
> > > line 
> > > should be accepted, otherwise it's just misplaced pickiness.
> > 
> > +1. Folks will just disable the warning if gcc is not very permissive
> > when paring existing comments. You cannot expect anyone to change
> > perfectly fine fall-through comments just to accommodate an arbitrary
> > gcc style.
> 
> The accepted style is already very permissive, we don't allow just one
> spelling as various lint tools.  I'm afraid looking for various cases of
> fall and through/thru possibly separated by anything and surrounded by
> anything is IMHO already too much, the compiler shouldn't try to try to
> grammar analyze the comments on what they actually talk about and whether it
> might be related to the switch fall through or something completely
> different.  Users should start using [[fallthrough]]; anyway.

Oh, forgot, I think allowing
  /* Fallthrough */
  /* arbitrary comment */
  case ...
might be something we could be also supporting, especially because
sometimes users might want to comment on what the following case handle and
fallthrough would be just something in between.  But IMHO forcing users to
use some clear markup style if they don't want to/can't switch to
[[fallthrough]];
__attribute__((fallthrough));
or some macro that does that is a good idea.  That will certainly increase
the chance other compilers could do the same thing, parsing arbitrary stuff
is hard to agree on.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 12:39:41PM +0200, Markus Trippelsdorf wrote:
> On 2016.09.27 at 10:46 +0200, Eric Botcazou wrote:
> > > The intent has been that we catch the most common forms, but still require
> > > it not to be complete free form.  Because, as experience shows, people are
> > > extremely creative in these comments, and it is not very good idea to
> > > support everything.  For ... fall through ... , what is the purpose of
> > > those ...s?
> > 
> > No idea, but it has been there for a while and seems perfectly reasonable.
> > IMO any sentence containing "fall" and "through/thru/etc" on the same line 
> > should be accepted, otherwise it's just misplaced pickiness.
> 
> +1. Folks will just disable the warning if gcc is not very permissive
> when paring existing comments. You cannot expect anyone to change
> perfectly fine fall-through comments just to accommodate an arbitrary
> gcc style.

The accepted style is already very permissive, we don't allow just one
spelling as various lint tools.  I'm afraid looking for various cases of
fall and through/thru possibly separated by anything and surrounded by
anything is IMHO already too much, the compiler shouldn't try to try to
grammar analyze the comments on what they actually talk about and whether it
might be related to the switch fall through or something completely
different.  Users should start using [[fallthrough]]; anyway.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 11:32:42AM +0100, Kyrill Tkachov wrote:
> where the code is:
> 2156   /* Fall through - if the lane index isn't a constant then
> 2157  the next case will error.  */
> 2158
> 2159 case NEON_ARG_CONSTANT:
> 
> 
> Is there supposed to be no empty line between the case statement and the 
> comment?
> Or is the comment only supposed to contain "Fall through"?

The last comment before case or default keyword (or user label before
case/default) has to match one of the following regexps:
//-fallthrough$
//@fallthrough@$
//[ \t]*FALL(S | |-)?THR(OUGH|U)\.?[ \t]*$
//[ \t]*Fall(s | |-)?[Tt]hr(ough|u)\.?[ \t]*$
//[ \t]*fall(s | |-)?thr(ough|u)\.?[ \t]*$
/\*-fallthrough\*/
/\*@fallthrough@\*/
/\*[ \t]*FALL(S | |-)?THR(OUGH|U)\.?[ \t]*\*/
/\*[ \t]*Fall(s | |-)?[Tt]hr(ough|u)\.?[ \t]*\*/
/\*[ \t]*fall(s | |-)?thr(ough|u)\.?[ \t]*\*/

So, you could e.g. write:
/* If the lane index isn't a constant, then the next case will error.  
*/
/* Fall through.  */
but not what you have, free form is not accepted.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Markus Trippelsdorf]

On 2016.09.27 at 10:46 +0200, Eric Botcazou wrote:
> > The intent has been that we catch the most common forms, but still require
> > it not to be complete free form.  Because, as experience shows, people are
> > extremely creative in these comments, and it is not very good idea to
> > support everything.  For ... fall through ... , what is the purpose of
> > those ...s?
> 
> No idea, but it has been there for a while and seems perfectly reasonable.
> IMO any sentence containing "fall" and "through/thru/etc" on the same line 
> should be accepted, otherwise it's just misplaced pickiness.

+1. Folks will just disable the warning if gcc is not very permissive
when paring existing comments. You cannot expect anyone to change
perfectly fine fall-through comments just to accommodate an arbitrary
gcc style.

-- 
Markus

Re: Implement -Wimplicit-fallthrough (version 9)

[Kyrill Tkachov]

Hi Marek,

On 27/09/16 10:44, Marek Polacek wrote:

On Tue, Sep 27, 2016 at 10:03:10AM +0200, Andreas Schwab wrote:

This breaks building with gcc-4.3.

g++ -std=gnu++98 -fno-PIE -c  -DUSE_LIBUNWIND_EXCEPTIONS  -g -DIN_GCC 
-fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wwrite-strings 
-Wcast-qual -Wno-format -Wmissing-format-attribute -Woverloaded-virtual 
-pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings  
-Wno-implicit-fallthrough -DHAVE_CONFIG_H -I. -I. -I../../gcc -I../../gcc/. 
-I../../gcc/../include -I../../gcc/../libcpp/include  
-I../../gcc/../libdecnumber -I../../gcc/../libdecnumber/dpd -I../libdecnumber 
-I../../gcc/../libbacktrace   -o insn-emit.o -MT insn-emit.o -MMD -MP -MF 
./.deps/insn-emit.TPo insn-emit.c
cc1plus: error: unrecognized command line option "-Wno-implicit-fallthrough"
make[3]: *** [insn-emit.o] Error 1

You're right, sorry.  Should be fixed in a bit.

Marek



I'm seeing about 4 triggers of this warning in the arm backend (thus breaking 
bootstrap),
 mostly due to the fall through comment adding some explanation.
For example:
$SRC/gcc/config/arm/arm-builtins.c: In function 'rtx_def* 
arm_expand_neon_args(rtx, machine_mode, int, int, int, tree, builtin_arg*)':
$SRC/gcc/config/arm/arm-builtins.c:2155:3: error: this statement may fall 
through [-Werror=implicit-fallthrough]
   }
   ^
$SRC/gcc/config/arm/arm-builtins.c:2159:6: note: here
  case NEON_ARG_CONSTANT:


where the code is:
2156   /* Fall through - if the lane index isn't a constant then
2157  the next case will error.  */
2158
2159 case NEON_ARG_CONSTANT:


Is there supposed to be no empty line between the case statement and the 
comment?
Or is the comment only supposed to contain "Fall through"?

Thanks,
Kyrill

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 10:03:10AM +0200, Andreas Schwab wrote:
> This breaks building with gcc-4.3.
> 
> g++ -std=gnu++98 -fno-PIE -c  -DUSE_LIBUNWIND_EXCEPTIONS  -g -DIN_GCC 
> -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall 
> -Wwrite-strings -Wcast-qual -Wno-format -Wmissing-format-attribute 
> -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros 
> -Wno-overlength-strings  -Wno-implicit-fallthrough -DHAVE_CONFIG_H -I. -I. 
> -I../../gcc -I../../gcc/. -I../../gcc/../include 
> -I../../gcc/../libcpp/include  -I../../gcc/../libdecnumber 
> -I../../gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc/../libbacktrace 
>   -o insn-emit.o -MT insn-emit.o -MMD -MP -MF ./.deps/insn-emit.TPo 
> insn-emit.c
> cc1plus: error: unrecognized command line option "-Wno-implicit-fallthrough"
> make[3]: *** [insn-emit.o] Error 1

You're right, sorry.  Should be fixed in a bit.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 27, 2016 at 09:50:17AM +0200, Gerald Pfeifer wrote:
> Hi Marek,
> 
> On Sat, 24 Sep 2016, Marek Polacek wrote:
> > All right.  I'll commit the patch on Monday.
> 
> my thrice a week bootstrap on old (but still "supported") 
> i?86-unknown-freebsd9 broke as follows:
> 
>   cc1plus: error: unrecognized command line option "-Wno-implicit-fallthrough"
>   gmake[3]: *** [Makefile:1102: insn-emit.o] Error 1
>   gmake[3]: *** Waiting for unfinished jobs
>   gmake[3]: Leaving directory '/scratch/tmp/gerald/OBJ-0927-0731/gcc'
>   gmake[2]: *** [Makefile:4571: all-stage1-gcc] Error 2
>   gmake[2]: Leaving directory '/scratch/tmp/gerald/OBJ-0927-0731'
>   gmake[1]: *** [Makefile:24240: stage1-bubble] Error 2
> 
> (Also filed as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77751 )

Sorry about that.  I just sent a fix.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Eric Botcazou]

> The intent has been that we catch the most common forms, but still require
> it not to be complete free form.  Because, as experience shows, people are
> extremely creative in these comments, and it is not very good idea to
> support everything.  For ... fall through ... , what is the purpose of
> those ...s?

No idea, but it has been there for a while and seems perfectly reasonable.
IMO any sentence containing "fall" and "through/thru/etc" on the same line 
should be accepted, otherwise it's just misplaced pickiness.

-- 
Eric Botcazou

Re: Implement -Wimplicit-fallthrough (version 9)

[Richard Biener]

On Tue, Sep 27, 2016 at 10:13 AM, Jakub Jelinek  wrote:
> On Tue, Sep 27, 2016 at 10:03:10AM +0200, Andreas Schwab wrote:
>> This breaks building with gcc-4.3.
>>
>> g++ -std=gnu++98 -fno-PIE -c  -DUSE_LIBUNWIND_EXCEPTIONS  -g -DIN_GCC 
>> -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall 
>> -Wwrite-strings -Wcast-qual -Wno-format -Wmissing-format-attribute 
>> -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros 
>> -Wno-overlength-strings  -Wno-implicit-fallthrough -DHAVE_CONFIG_H -I. -I. 
>> -I../../gcc -I../../gcc/. -I../../gcc/../include 
>> -I../../gcc/../libcpp/include  -I../../gcc/../libdecnumber 
>> -I../../gcc/../libdecnumber/dpd -I../libdecnumber 
>> -I../../gcc/../libbacktrace   -o insn-emit.o -MT insn-emit.o -MMD -MP -MF 
>> ./.deps/insn-emit.TPo insn-emit.c
>> cc1plus: error: unrecognized command line option "-Wno-implicit-fallthrough"
>> make[3]: *** [insn-emit.o] Error 1
>
> Guess it must have been some mistake against the policy that unknown -Wno-*
> options aren't complained about, unless something else is diagnosed.
>
> Anyway, Marek, can you add configure check for that?

Existing practice is to instead use -Wno-error rather than warning
flags that were not present in those early GCC versions.

Richard.

> Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 08:49:00AM +0200, Eric Botcazou wrote:
> > It seems unfortunate that the warning doesn't accept /* ... fall
> > through ... */ as a fallthrough comment.
> 
> Seconded.  The warning should take into account existing practices instead of 
> forcing the user to make completely bogus changes to the code (and Ada should 
> have been tested before the patch was approved).

The intent has been that we catch the most common forms, but still require it 
not
to be complete free form.  Because, as experience shows, people are
extremely creative in these comments, and it is not very good idea to
support everything.  For ... fall through ... , what is the purpose of those
...s?

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Tue, Sep 27, 2016 at 10:03:10AM +0200, Andreas Schwab wrote:
> This breaks building with gcc-4.3.
> 
> g++ -std=gnu++98 -fno-PIE -c  -DUSE_LIBUNWIND_EXCEPTIONS  -g -DIN_GCC 
> -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall 
> -Wwrite-strings -Wcast-qual -Wno-format -Wmissing-format-attribute 
> -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros 
> -Wno-overlength-strings  -Wno-implicit-fallthrough -DHAVE_CONFIG_H -I. -I. 
> -I../../gcc -I../../gcc/. -I../../gcc/../include 
> -I../../gcc/../libcpp/include  -I../../gcc/../libdecnumber 
> -I../../gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc/../libbacktrace 
>   -o insn-emit.o -MT insn-emit.o -MMD -MP -MF ./.deps/insn-emit.TPo 
> insn-emit.c
> cc1plus: error: unrecognized command line option "-Wno-implicit-fallthrough"
> make[3]: *** [insn-emit.o] Error 1

Guess it must have been some mistake against the policy that unknown -Wno-*
options aren't complained about, unless something else is diagnosed.

Anyway, Marek, can you add configure check for that?

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Andreas Schwab]

This breaks building with gcc-4.3.

g++ -std=gnu++98 -fno-PIE -c  -DUSE_LIBUNWIND_EXCEPTIONS  -g -DIN_GCC 
-fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wwrite-strings 
-Wcast-qual -Wno-format -Wmissing-format-attribute -Woverloaded-virtual 
-pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings  
-Wno-implicit-fallthrough -DHAVE_CONFIG_H -I. -I. -I../../gcc -I../../gcc/. 
-I../../gcc/../include -I../../gcc/../libcpp/include  
-I../../gcc/../libdecnumber -I../../gcc/../libdecnumber/dpd -I../libdecnumber 
-I../../gcc/../libbacktrace   -o insn-emit.o -MT insn-emit.o -MMD -MP -MF 
./.deps/insn-emit.TPo insn-emit.c
cc1plus: error: unrecognized command line option "-Wno-implicit-fallthrough"
make[3]: *** [insn-emit.o] Error 1

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Implement -Wimplicit-fallthrough (version 9)

[Eric Botcazou]

> Seconded.  The warning should take into account existing practices instead
> of forcing the user to make completely bogus changes to the code (and Ada
> should have been tested before the patch was approved).

I have a bootstrap failure on x86-64/Linux:

/home/eric/svn/gcc/gcc/combine.c: In function 'rtx_code 
simplify_comparison(rtx_code, rtx_def**, rtx_def**)':
/home/eric/svn/gcc/gcc/combine.c:11928:11: error: this statement may fall 
through [-Werror=implicit-fallthrough]
  break;
   ^
/home/eric/svn/gcc/gcc/combine.c:11932:2: note: here
  case ZERO_EXTEND:
  ^~~~
/home/eric/svn/gcc/gcc/combine.c:12340:6: error: this statement may fall 
through [-Werror=implicit-fallthrough]
  }
  ^
/home/eric/svn/gcc/gcc/combine.c:12343:2: note: here
  case LSHIFTRT:
  ^~~~

  /* ... fall through ...  */

  /* ... fall through ...  */

-- 
Eric Botcazou

Re: Implement -Wimplicit-fallthrough (version 9)

[Gerald Pfeifer]

Hi Marek,

On Sat, 24 Sep 2016, Marek Polacek wrote:
> All right.  I'll commit the patch on Monday.

my thrice a week bootstrap on old (but still "supported") 
i?86-unknown-freebsd9 broke as follows:

  cc1plus: error: unrecognized command line option "-Wno-implicit-fallthrough"
  gmake[3]: *** [Makefile:1102: insn-emit.o] Error 1
  gmake[3]: *** Waiting for unfinished jobs
  gmake[3]: Leaving directory '/scratch/tmp/gerald/OBJ-0927-0731/gcc'
  gmake[2]: *** [Makefile:4571: all-stage1-gcc] Error 2
  gmake[2]: Leaving directory '/scratch/tmp/gerald/OBJ-0927-0731'
  gmake[1]: *** [Makefile:24240: stage1-bubble] Error 2

(Also filed as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77751 )

Gerald

Re: Implement -Wimplicit-fallthrough (version 9)

[Eric Botcazou]

> It seems unfortunate that the warning doesn't accept /* ... fall
> through ... */ as a fallthrough comment.

Seconded.  The warning should take into account existing practices instead of 
forcing the user to make completely bogus changes to the code (and Ada should 
have been tested before the patch was approved).

-- 
Eric Botcazou

Re: Implement -Wimplicit-fallthrough (version 9)

[Rainer Orth]

Hi Marek,

> On Mon, Sep 26, 2016 at 03:34:40PM +0200, Rainer Orth wrote:
>> Hi Marek,
>> 
>> > All right.  I'll commit the patch on Monday.
>> 
>> this patch caused quite some breakage: Ada, Solaris/x86 and SPARC don't
>> bootstrap any longer. 
>  
> Sorry about that.  I had tested Ada + x86_64/ppc64/aarch64, but couldn't
> test neither Solaris nor SPARC.

the Solaris/x86 and SPARC issues were minor; I was just astonished to
see that so much didn't work on the Ada side.

>> The following patch allows i386-pc-solaris2.12 and sparc-sun-solaris2.12
>> bootstraps continue.
>> 
>> Strangely, I needed to use gcc_fallthrough () in i386.c; a mere /* FALLTHRU 
>> */
>> had no effect.
>  
> Yes, this is expected.  /* FALLTHRU */ comments only work if the next token
> is a case label or default label.

Ok, that explains what's going on.  I guess it would be good to make
this explicit in invoke.texi since someone else is guaranteed to stumble
across this as well.

Thanks.
Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Implement -Wimplicit-fallthrough (version 9)

[Rainer Orth]

Hi Jakub,

> On Mon, Sep 26, 2016 at 03:34:40PM +0200, Rainer Orth wrote:
>> Ok for mainline if the bootstraps pass (with appropriate changelog
>> entries, of course)?
>
> Yes.

testing completed successfully, so I've installed the patch with this
ChangeLog entry:

2016-09-26  Rainer Orth  

gcc:
* config/i386/i386.c (ix86_print_operand)
[HAVE_AS_IX86_CMOV_SUN_SYNTAX]: Add gcc_fallthrough.
* config/sparc/sparc.c (check_pic): Add fallthrough comment.
(epilogue_renumber): Likewise.

gcc/ada:
* gcc-interface/decl.c: Fix fall through comment formatting.
* gcc-interface/misc.c: Likewise.
* gcc-interface/trans.c: Likewise.
* gcc-interface/utils.c: Likewise.
* gcc-interface/utils2.c: Likewise.

>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -17917,6 +17917,7 @@ ix86_print_operand (FILE *file, rtx x, i
>>  #ifdef HAVE_AS_IX86_CMOV_SUN_SYNTAX
>>if (ASSEMBLER_DIALECT == ASM_ATT)
>>  putc ('.', file);
>> +  gcc_fallthrough ();
>>  #endif
>
> Where have you been adding the /*FALLTHROUGH*/ ?  Before #endif or after it?

Before: seemed natural to me.

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Implement -Wimplicit-fallthrough (version 9)

[Jason Merrill]

It seems unfortunate that the warning doesn't accept /* ... fall
through ... */ as a fallthrough comment.

Jason

Re: Implement -Wimplicit-fallthrough (version 9)

[Arnaud Charlet]

> this patch caused quite some breakage: Ada, Solaris/x86 and SPARC don't
> bootstrap any longer. 
> 
> The following patch allows i386-pc-solaris2.12 and
> sparc-sun-solaris2.12
> bootstraps continue.
> 
> Strangely, I needed to use gcc_fallthrough () in i386.c; a mere /* FALLTHRU
> */
> had no effect.
> 
> Ok for mainline if the bootstraps pass (with appropriate changelog
> entries, of course)?

Ada part is OK

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Mon, Sep 26, 2016 at 03:34:40PM +0200, Rainer Orth wrote:
> Hi Marek,
> 
> > All right.  I'll commit the patch on Monday.
> 
> this patch caused quite some breakage: Ada, Solaris/x86 and SPARC don't
> bootstrap any longer. 
 
Sorry about that.  I had tested Ada + x86_64/ppc64/aarch64, but couldn't
test neither Solaris nor SPARC.

> The following patch allows i386-pc-solaris2.12 and sparc-sun-solaris2.12
> bootstraps continue.
> 
> Strangely, I needed to use gcc_fallthrough () in i386.c; a mere /* FALLTHRU */
> had no effect.
 
Yes, this is expected.  /* FALLTHRU */ comments only work if the next token
is a case label or default label.

> Ok for mainline if the bootstraps pass (with appropriate changelog
> entries, of course)?

LGTM, though I can't approve.

Thanks a lot.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Mon, Sep 26, 2016 at 03:34:40PM +0200, Rainer Orth wrote:
> Ok for mainline if the bootstraps pass (with appropriate changelog
> entries, of course)?

Yes.

> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -17917,6 +17917,7 @@ ix86_print_operand (FILE *file, rtx x, i
>  #ifdef HAVE_AS_IX86_CMOV_SUN_SYNTAX
> if (ASSEMBLER_DIALECT == ASM_ATT)
>   putc ('.', file);
> +   gcc_fallthrough ();
>  #endif

Where have you been adding the /*FALLTHROUGH*/ ?  Before #endif or after it?

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Rainer Orth]

Hi Marek,

> All right.  I'll commit the patch on Monday.

this patch caused quite some breakage: Ada, Solaris/x86 and SPARC don't
bootstrap any longer. 

The following patch allows i386-pc-solaris2.12 and sparc-sun-solaris2.12
bootstraps continue.

Strangely, I needed to use gcc_fallthrough () in i386.c; a mere /* FALLTHRU */
had no effect.

Ok for mainline if the bootstraps pass (with appropriate changelog
entries, of course)?

Rainer


diff --git a/gcc/ada/gcc-interface/decl.c b/gcc/ada/gcc-interface/decl.c
--- a/gcc/ada/gcc-interface/decl.c
+++ b/gcc/ada/gcc-interface/decl.c
@@ -596,7 +596,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit
 	gnu_expr
 	  = gnat_to_gnu_external (Expression (Declaration_Node (gnat_entity)));
 
-  /* ... fall through ... */
+  /* fall through */
 
 case E_Exception:
 case E_Loop_Parameter:
@@ -3369,7 +3369,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit
 	  break;
 	}
 
-  /* ... fall through ... */
+  /* fall through */
 
 case E_Record_Subtype:
   /* If Cloned_Subtype is Present it means this record subtype has
@@ -3804,7 +3804,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit
 	break;
 	}
 
-  /* ... fall through ... */
+  /* fall through */
 
 case E_Allocator_Type:
 case E_Access_Type:
@@ -6882,7 +6882,7 @@ choices_to_gnu (tree operand, Node_Id ch
 	  break;
 	}
 
-	  /* ... fall through ... */
+	  /* fall through */
 
 	case N_Character_Literal:
 	case N_Integer_Literal:
@@ -8089,7 +8089,7 @@ annotate_value (tree gnu_size)
   else
 	return Uint_Minus_1;
 
-  /* Fall through... */
+  /* fall through */
 
 default:
   return No_Uint;
diff --git a/gcc/ada/gcc-interface/misc.c b/gcc/ada/gcc-interface/misc.c
--- a/gcc/ada/gcc-interface/misc.c
+++ b/gcc/ada/gcc-interface/misc.c
@@ -157,7 +157,7 @@ gnat_handle_option (size_t scode, const 
 case OPT_gant:
   warning (0, "%<-gnat%> misspelled as %<-gant%>");
 
-  /* ... fall through ... */
+  /* fall through */
 
 case OPT_gnat:
 case OPT_gnatO:
@@ -486,13 +486,13 @@ gnat_print_type (FILE *file, tree node, 
   else
 	print_node (file, "index type", TYPE_INDEX_TYPE (node), indent + 4);
 
-  /* ... fall through ... */
+  /* fall through */
 
 case ENUMERAL_TYPE:
 case BOOLEAN_TYPE:
   print_node_brief (file, "RM size", TYPE_RM_SIZE (node), indent + 4);
 
-  /* ... fall through ... */
+  /* fall through */
 
 case REAL_TYPE:
   print_node_brief (file, "RM min", TYPE_RM_MIN_VALUE (node), indent + 4);
diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
--- a/gcc/ada/gcc-interface/trans.c
+++ b/gcc/ada/gcc-interface/trans.c
@@ -844,7 +844,7 @@ lvalue_required_p (Node_Id gnat_node, tr
 		 && Ekind (Entity (gnat_temp)) == E_Enumeration_Literal))
 	  return 1;
 
-  /* ... fall through ... */
+  /* fall through */
 
 case N_Slice:
   /* Only the array expression can require an lvalue.  */
@@ -890,7 +890,7 @@ lvalue_required_p (Node_Id gnat_node, tr
 	if (!constant)
 	  return 1;
 
-  /* ... fall through ... */
+  /* fall through */
 
 case N_Type_Conversion:
 case N_Qualified_Expression:
@@ -914,7 +914,7 @@ lvalue_required_p (Node_Id gnat_node, tr
   get_unpadded_type (Etype (gnat_parent)),
   true, false, true);
 
-  /* ... fall through ... */
+  /* fall through */
 
 default:
   return 0;
@@ -1681,7 +1681,7 @@ Attribute_to_gnu (Node_Id gnat_node, tre
 	  break;
 	}
 
-  /* ... fall through ... */
+  /* fall through */
 
 case Attr_Access:
 case Attr_Unchecked_Access:
@@ -1938,7 +1938,7 @@ Attribute_to_gnu (Node_Id gnat_node, tre
 	  break;
 	}
 
-  /* ... fall through ... */
+  /* fall through */
 
 case Attr_Length:
   {
@@ -2393,7 +2393,7 @@ Attribute_to_gnu (Node_Id gnat_node, tre
   /* We treat Model as identical to Machine.  This is true for at least
 	 IEEE and some other nice floating-point systems.  */
 
-  /* ... fall through ... */
+  /* fall through */
 
 case Attr_Machine:
   /* The trick is to force the compiler to store the result in memory so
@@ -2537,7 +2537,7 @@ Case_Statement_to_gnu (Node_Id gnat_node
 		  break;
 		}
 
-	  /* ... fall through ... */
+	  /* fall through */
 
 	case N_Character_Literal:
 	case N_Integer_Literal:
@@ -4007,7 +4007,7 @@ node_is_atomic (Node_Id gnat_node)
 	  && Has_Atomic_Components (Entity (Prefix (gnat_node
 	return true;
 
-  /* ... fall through ... */
+  /* fall through */
 
 case N_Explicit_Dereference:
   return Is_Atomic (Etype (gnat_node));
@@ -4123,7 +4123,7 @@ atomic_access_required_p (Node_Id gnat_n
   /* Nothing to do if we are the prefix of an attribute, since we do not
 	 want an atomic access for things like 'Size.  */
 
-  /* ... fall through ... */
+  /* fall through */
 
 case N_Reference:
   /* The N_Reference node is like an attribute.  */
@@ 

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Fri, Sep 23, 2016 at 10:12:00PM +, Joseph Myers wrote:
> On Fri, 23 Sep 2016, Marek Polacek wrote:
> 
> > On Fri, Sep 23, 2016 at 10:22:15AM -0400, Jason Merrill wrote:
> > > On Thu, Sep 22, 2016 at 9:59 AM, Marek Polacek  wrote:
> > > >> This is very close, thanks.  Let's give a more helpful warning about
> > > >>
> > > >> [[fallthrough]] 0;
> > > >> __attribute__ ((fallthrough)) 0;
> > > >>
> > > >> both here and in cp_parser_statement, something like "fallthrough 
> > > >> attribute
> > > >> not followed by ';'"
> > > >
> > > > Done.  And I made similar tweaks in the C FE.
> > > 
> > > The C++ changes are OK.
> > 
> > Thanks very much!
> > 
> > Now I'd like to ask for an approval of the ME parts.
> > 
> > Also, Joseph said he had no comments, but since I've changed the C FE
> > parts a bit, I'd like to give him a chance to comment to perhaps look
> > at this again.
> 
> I still have no comments on the C changes.

All right.  I'll commit the patch on Monday.

Thanks all.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Joseph Myers]

On Fri, 23 Sep 2016, Marek Polacek wrote:

> On Fri, Sep 23, 2016 at 10:22:15AM -0400, Jason Merrill wrote:
> > On Thu, Sep 22, 2016 at 9:59 AM, Marek Polacek  wrote:
> > >> This is very close, thanks.  Let's give a more helpful warning about
> > >>
> > >> [[fallthrough]] 0;
> > >> __attribute__ ((fallthrough)) 0;
> > >>
> > >> both here and in cp_parser_statement, something like "fallthrough 
> > >> attribute
> > >> not followed by ';'"
> > >
> > > Done.  And I made similar tweaks in the C FE.
> > 
> > The C++ changes are OK.
> 
> Thanks very much!
> 
> Now I'd like to ask for an approval of the ME parts.
> 
> Also, Joseph said he had no comments, but since I've changed the C FE
> parts a bit, I'd like to give him a chance to comment to perhaps look
> at this again.

I still have no comments on the C changes.

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

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Fri, Sep 23, 2016 at 04:23:29PM +0200, Jakub Jelinek wrote:
> On Fri, Sep 23, 2016 at 10:22:15AM -0400, Jason Merrill wrote:
> > On Thu, Sep 22, 2016 at 9:59 AM, Marek Polacek  wrote:
> > >> This is very close, thanks.  Let's give a more helpful warning about
> > >>
> > >> [[fallthrough]] 0;
> > >> __attribute__ ((fallthrough)) 0;
> > >>
> > >> both here and in cp_parser_statement, something like "fallthrough 
> > >> attribute
> > >> not followed by ';'"
> > >
> > > Done.  And I made similar tweaks in the C FE.
> > 
> > The C++ changes are OK.
> 
> And the middle-end changes are ok too.

Thanks a lot.  As mentioned in the other mail I've just sent, I'll give
Joseph a chance to comment on this, if he wishes to.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Fri, Sep 23, 2016 at 10:22:15AM -0400, Jason Merrill wrote:
> On Thu, Sep 22, 2016 at 9:59 AM, Marek Polacek  wrote:
> >> This is very close, thanks.  Let's give a more helpful warning about
> >>
> >> [[fallthrough]] 0;
> >> __attribute__ ((fallthrough)) 0;
> >>
> >> both here and in cp_parser_statement, something like "fallthrough attribute
> >> not followed by ';'"
> >
> > Done.  And I made similar tweaks in the C FE.
> 
> The C++ changes are OK.

Thanks very much!

Now I'd like to ask for an approval of the ME parts.

Also, Joseph said he had no comments, but since I've changed the C FE
parts a bit, I'd like to give him a chance to comment to perhaps look
at this again.

Thanks,

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Jakub Jelinek]

On Fri, Sep 23, 2016 at 10:22:15AM -0400, Jason Merrill wrote:
> On Thu, Sep 22, 2016 at 9:59 AM, Marek Polacek  wrote:
> >> This is very close, thanks.  Let's give a more helpful warning about
> >>
> >> [[fallthrough]] 0;
> >> __attribute__ ((fallthrough)) 0;
> >>
> >> both here and in cp_parser_statement, something like "fallthrough attribute
> >> not followed by ';'"
> >
> > Done.  And I made similar tweaks in the C FE.
> 
> The C++ changes are OK.

And the middle-end changes are ok too.

Jakub

Re: Implement -Wimplicit-fallthrough (version 9)

[Jason Merrill]

On Thu, Sep 22, 2016 at 9:59 AM, Marek Polacek  wrote:
>> This is very close, thanks.  Let's give a more helpful warning about
>>
>> [[fallthrough]] 0;
>> __attribute__ ((fallthrough)) 0;
>>
>> both here and in cp_parser_statement, something like "fallthrough attribute
>> not followed by ';'"
>
> Done.  And I made similar tweaks in the C FE.

The C++ changes are OK.

Jason

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

It occurred to me that I should also handle the
__has_cpp_attribute(fallthrough) part.  But I hope that can be done after the
main -Wimplicit-fallthrough is committed.

Marek

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Wed, Sep 21, 2016 at 03:14:07PM -0400, Jason Merrill wrote:
> On 09/21/2016 02:59 PM, Marek Polacek wrote:
> > +  if (statement == NULL_TREE
> > +  && attr != NULL_TREE
> > +  && maybe_attribute_fallthrough_p (attr))
> > +{
> > +  /* Turn [[fallthrough]]; into FALLTHROUGH ();.  */
> > +  statement = build_call_expr_internal_loc (loc, IFN_FALLTHROUGH,
> > +   void_type_node, 0);
> > +  attr = NULL_TREE;
> > +}
> > +
> > +  /* Allow "[[fallthrough]];", but warn otherwise.  */
> > +  if (attr != NULL_TREE)
> > +warning_at (loc, OPT_Wattributes,
> > +   "attributes at the beginning of statement are ignored");
> 
> This is very close, thanks.  Let's give a more helpful warning about
> 
> [[fallthrough]] 0;
> __attribute__ ((fallthrough)) 0;
> 
> both here and in cp_parser_statement, something like "fallthrough attribute
> not followed by ';'"

Done.  And I made similar tweaks in the C FE.

Bootstrapped/regtested on x86_64-linux and ppc64-linux.

2016-09-22  Marek Polacek  
Jakub Jelinek  

PR c/7652
gcc/
* 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.
gcc/c-family/
* c-common.c (c_common_attribute_table): Add fallthrough attribute.
(handle_fallthrough_attribute): New function.
(attribute_fallthrough_p): New function.
* c-common.h (attribute_fallthrough_p): Declare.
gcc/c/
* c-parser.c (struct c_token): Add flags field.
(c_lex_one_token): Pass it to c_lex_with_flags.
(c_parser_declaration_or_fndef): Turn __attribute__((fallthrough));
into IFN_FALLTHROUGH.
(c_parser_label): Set FALLTHROUGH_LABEL_P on labels.  Handle
attribute fallthrough after a case label or default label.
(c_parser_statement_after_labels): Handle RID_ATTRIBUTE.
gcc/cp/
* constexpr.c (cxx_eval_internal_function): Handle IFN_FALLTHROUGH.
(potential_constant_expression_1): Likewise.
* constraint.cc (function_concept_check_p): Check fn for null.
* parser.c (cp_parser_expression_statement): Handle attribute
fallthrough.
(cp_parser_statement): Likewise.
(cp_parser_label_for_labeled_statement): Set FALLTHROUGH_LABEL_P on
labels.
(cp_parser_std_attribute): Handle fallthrough attribute.
(cp_parser_check_std_attribute): Add %< %> quotes.
* pt.c (tsubst_copy_and_build): Handle internal functions.
(instantiation_dependent_scope_ref_p): Return if the expression is
null.
gcc/testsuite/
* c-c++-common/Wimplicit-fallthrough-1.c: New test.
* c-c++-common/Wimplicit-fallthrough-10.c: New test.
* c-c++-common/Wimplicit-fallthrough-11.c: New test.
* c-c++-common/Wimplicit-fallthrough-12.c: New test.
* c-c++-common/Wimplicit-fallthrough-13.c: New test.
* c-c++-common/Wimplicit-fallthrough-14.c: New test.
* c-c++-common/Wimplicit-fallthrough-15.c: New test.
* c-c++-common/Wimplicit-fallthrough-16.c: New test.
* c-c++-common/Wimplicit-fallthrough-17.c: New test.
* c-c++-common/Wimplicit-fallthrough-18.c: New test.
* c-c++-common/Wimplicit-fallthrough-19.c: New test.
* c-c++-common/Wimplicit-fallthrough-20.c: New test.
* c-c++-common/Wimplicit-fallthrough-21.c: New test.
* c-c++-common/Wimplicit-fallthrough-2.c: New test.
* c-c++-common/Wimplicit-fallthrough-3.c: New test.
* c-c++-common/Wimplicit-fallthrough-4.c: New 

Re: Implement -Wimplicit-fallthrough (version 9)

[Jason Merrill]

On 09/21/2016 02:59 PM, Marek Polacek wrote:

+  if (statement == NULL_TREE
+  && attr != NULL_TREE
+  && maybe_attribute_fallthrough_p (attr))
+{
+  /* Turn [[fallthrough]]; into FALLTHROUGH ();.  */
+  statement = build_call_expr_internal_loc (loc, IFN_FALLTHROUGH,
+   void_type_node, 0);
+  attr = NULL_TREE;
+}
+
+  /* Allow "[[fallthrough]];", but warn otherwise.  */
+  if (attr != NULL_TREE)
+warning_at (loc, OPT_Wattributes,
+   "attributes at the beginning of statement are ignored");


This is very close, thanks.  Let's give a more helpful warning about

[[fallthrough]] 0;
__attribute__ ((fallthrough)) 0;

both here and in cp_parser_statement, something like "fallthrough 
attribute not followed by ';'"


Jason

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Wed, Sep 21, 2016 at 10:06:07AM -0400, Jason Merrill wrote:
> On 09/21/2016 08:44 AM, Marek Polacek wrote:
> > @@ -10733,12 +10758,35 @@ cp_parser_expression_statement (cp_parser* 
> > parser, tree in_statement_expr)
> >   statement.  */
> >if (cp_lexer_next_token_is_not (parser->lexer, CPP_SEMICOLON))
> >  {
> > +  /* This might be attribute fallthrough.  */
> > +  if (cp_lexer_next_token_is_keyword (parser->lexer, RID_ATTRIBUTE))
> > {
> > + tree attr = cp_parser_gnu_attributes_opt (parser);
> > + if (maybe_attribute_fallthrough_p (attr))
> > +   statement = build_call_expr_internal_loc (token->location,
> > + IFN_FALLTHROUGH,
> > + void_type_node, 0);
> > + else if (cp_lexer_next_token_is (parser->lexer, CPP_SEMICOLON))
> > +   {
> > + cp_parser_error (parser, "only attribute % "
> > +  "can be applied to a null statement");
> > + return error_mark_node;
> > +   }
> > + else
> > +   {
> > + cp_parser_error (parser, "expected primary-expression");
> > + return error_mark_node;
> > +   }
> > +   }
> 
> Attributes that we don't handle should be warnings, not errors.  Like we do
> for C++11 attributes in cp_parser_statement, we should first parse
> attributes, then parse the expression-statement, then give any appropriate
> warnings.

Ok, I hope this is at least somewhat closer to what you imagine.  I had to
adjust g++.dg/warn/Wunused-label-1.C test because of this.  I also turned
errors for e.g. __attribute__((used)); into warnings in the C FE.

So hopefully one step closer.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-09-21  Marek Polacek  
Jakub Jelinek  

PR c/7652
gcc/
* 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.
gcc/c-family/
* c-common.c (c_common_attribute_table): Add fallthrough attribute.
(handle_fallthrough_attribute): New function.
(maybe_attribute_fallthrough_p): New function.
* c-common.h (maybe_attribute_fallthrough_p): Declare.
gcc/c/
* c-parser.c (struct c_token): Add flags field.
(c_lex_one_token): Pass it to c_lex_with_flags.
(c_parser_declaration_or_fndef): Turn __attribute__((fallthrough));
into IFN_FALLTHROUGH.
(c_parser_label): Set FALLTHROUGH_LABEL_P on labels.  Handle
attribute fallthrough after a case label or default label.
(c_parser_statement_after_labels): Handle RID_ATTRIBUTE.
gcc/cp/
* constexpr.c (cxx_eval_internal_function): Handle IFN_FALLTHROUGH.
(potential_constant_expression_1): Likewise.
* constraint.cc (function_concept_check_p): Check fn for null.
* parser.c (cp_parser_expression_statement): Handle attribute
fallthrough.
(cp_parser_statement): Likewise.
(cp_parser_label_for_labeled_statement): Set FALLTHROUGH_LABEL_P on
labels.
(cp_parser_std_attribute): Handle fallthrough attribute.
(cp_parser_check_std_attribute): Add %< %> quotes.
* pt.c (tsubst_copy_and_build): Handle internal functions.
(instantiation_dependent_scope_ref_p): Return if the expression is
null.
gcc/testsuite/
* c-c++-common/Wimplicit-fallthrough-1.c: New test.
* c-c++-common/Wimplicit-fallthrough-10.c: New test.
* c-c++-common/Wimplicit-fallthrough-11.c: New test.
* 

Re: Implement -Wimplicit-fallthrough (version 9)

[Jason Merrill]

On 09/21/2016 08:44 AM, Marek Polacek wrote:

@@ -10733,12 +10758,35 @@ cp_parser_expression_statement (cp_parser* parser, 
tree in_statement_expr)
  statement.  */
   if (cp_lexer_next_token_is_not (parser->lexer, CPP_SEMICOLON))
 {
+  /* This might be attribute fallthrough.  */
+  if (cp_lexer_next_token_is_keyword (parser->lexer, RID_ATTRIBUTE))
{
+ tree attr = cp_parser_gnu_attributes_opt (parser);
+ if (maybe_attribute_fallthrough_p (attr))
+   statement = build_call_expr_internal_loc (token->location,
+ IFN_FALLTHROUGH,
+ void_type_node, 0);
+ else if (cp_lexer_next_token_is (parser->lexer, CPP_SEMICOLON))
+   {
+ cp_parser_error (parser, "only attribute % "
+  "can be applied to a null statement");
+ return error_mark_node;
+   }
+ else
+   {
+ cp_parser_error (parser, "expected primary-expression");
+ return error_mark_node;
+   }
+   }


Attributes that we don't handle should be warnings, not errors.  Like we 
do for C++11 attributes in cp_parser_statement, we should first parse 
attributes, then parse the expression-statement, then give any 
appropriate warnings.


Jason

Re: Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

On Tue, Sep 20, 2016 at 12:26:11PM -0400, Jason Merrill wrote:
> On 09/20/2016 11:33 AM, Marek Polacek wrote:
> > @@ -5135,6 +5135,30 @@ cp_parser_primary_expression (cp_parser *parser,
> > case RID_AT_SELECTOR:
> >   return cp_parser_objc_expression (parser);
> > 
> > +   case RID_ATTRIBUTE:
> 
> Attribute handling doesn't belong in this function; we don't want to allow
> attributes anywhere we see a primary-expression.  This should be either in
> cp_parser_expression_statement or cp_parser_statement.

All right.  I moved the attribute handling to cp_parser_expression_statement
in the following patch.  Anything else?  Thanks,

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-09-21  Marek Polacek  
Jakub Jelinek  

PR c/7652
gcc/
* 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.
gcc/c-family/
* c-common.c (c_common_attribute_table): Add fallthrough attribute.
(handle_fallthrough_attribute): New function.
(maybe_attribute_fallthrough_p): New function.
* c-common.h (maybe_attribute_fallthrough_p): Declare.
gcc/c/
* c-parser.c (struct c_token): Add flags field.
(c_lex_one_token): Pass it to c_lex_with_flags.
(c_parser_declaration_or_fndef): Turn __attribute__((fallthrough));
into IFN_FALLTHROUGH.
(c_parser_label): Set FALLTHROUGH_LABEL_P on labels.  Handle
attribute fallthrough after a case label or default label.
(c_parser_statement_after_labels): Handle RID_ATTRIBUTE.
gcc/cp/
* constexpr.c (cxx_eval_internal_function): Handle IFN_FALLTHROUGH.
(potential_constant_expression_1): Likewise.
* constraint.cc (function_concept_check_p): Check fn for null.
* parser.c (cp_parser_expression_statement): Handle RID_ATTRIBUTE.
(cp_parser_statement): Handle fallthrough attribute.
(cp_parser_label_for_labeled_statement): Set FALLTHROUGH_LABEL_P on
labels.
(cp_parser_std_attribute): Handle fallthrough attribute.
(cp_parser_check_std_attribute): Add %< %> quotes.
* pt.c (tsubst_copy_and_build): Handle internal functions.
(instantiation_dependent_scope_ref_p): Return if the expression is
null.
gcc/testsuite/
* c-c++-common/Wimplicit-fallthrough-1.c: New test.
* c-c++-common/Wimplicit-fallthrough-10.c: New test.
* c-c++-common/Wimplicit-fallthrough-11.c: New test.
* c-c++-common/Wimplicit-fallthrough-12.c: New test.
* c-c++-common/Wimplicit-fallthrough-13.c: New test.
* c-c++-common/Wimplicit-fallthrough-14.c: New test.
* c-c++-common/Wimplicit-fallthrough-15.c: New test.
* c-c++-common/Wimplicit-fallthrough-16.c: New test.
* c-c++-common/Wimplicit-fallthrough-17.c: New test.
* c-c++-common/Wimplicit-fallthrough-18.c: New test.
* c-c++-common/Wimplicit-fallthrough-19.c: New test.
* c-c++-common/Wimplicit-fallthrough-20.c: New test.
* c-c++-common/Wimplicit-fallthrough-21.c: New test.
* c-c++-common/Wimplicit-fallthrough-2.c: New test.
* c-c++-common/Wimplicit-fallthrough-3.c: New test.
* c-c++-common/Wimplicit-fallthrough-4.c: New test.
* c-c++-common/Wimplicit-fallthrough-5.c: New test.
* c-c++-common/Wimplicit-fallthrough-6.c: New test.
* c-c++-common/Wimplicit-fallthrough-7.c: New test.
* c-c++-common/Wimplicit-fallthrough-8.c: New test.
* c-c++-common/Wimplicit-fallthrough-9.c: New test.
* 

Re: Implement -Wimplicit-fallthrough (version 9)

[Jason Merrill]

On 09/20/2016 11:33 AM, Marek Polacek wrote:

@@ -5135,6 +5135,30 @@ cp_parser_primary_expression (cp_parser *parser,
case RID_AT_SELECTOR:
  return cp_parser_objc_expression (parser);

+   case RID_ATTRIBUTE:


Attribute handling doesn't belong in this function; we don't want to 
allow attributes anywhere we see a primary-expression.  This should be 
either in cp_parser_expression_statement or cp_parser_statement.


Jason

Implement -Wimplicit-fallthrough (version 9)

[Marek Polacek]

Time for another latest & greatest version of -Wimplicit-fallthrough.
Hope this time it's near its end.

Changes from last time:

* I addressed Jakub's comment, for details see
  
* in the C FE:
  case 0:
__attribute)) ((fallthrough));
  case 1:
...
  now works
* we've discussed cases like
  [[fallthrough]]; lab1: case 1:;
  and
  [[fallthrough]]; lab2:; case 2:;

  This was discussed on the C++ committee reflector and it was concluded that
  the former should work and the latter should not.  So I implemented that.
  But there's no way to tell the difference between "lab1: case 1:;" and
  "lab2:; case 2:;" so the null statements are ignored.  Another problem is
  what to do with "[[fallthrough]]; lab1: (void) 0; case 2:".  Currently it's
  not rejected because we optimize "(void) 0;" out in gimplify_expr.  And I
  couldn't figure out how to circumvent this (in gimplify_expr you can't see
  what the next statement is).  Though this seems more like a pedantic issue
  and probably not a real-world issue.  (E.g.
  "[[fallthrough]]; lab1: if (0) { ... } case 2:"
  should be rejected.)
 
Jason, would you mind looking at the C++ FE parts again?

Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for trunk?

2016-09-20  Marek Polacek  
Jakub Jelinek  

PR c/7652
gcc/
* 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.
gcc/c-family/
* c-common.c (c_common_attribute_table): Add fallthrough attribute.
(handle_fallthrough_attribute): New function.
(maybe_attribute_fallthrough_p): New function.
* c-common.h (maybe_attribute_fallthrough_p): Declare.
gcc/c/
* c-parser.c (struct c_token): Add flags field.
(c_lex_one_token): Pass it to c_lex_with_flags.
(c_parser_declaration_or_fndef): Turn __attribute__((fallthrough));
into IFN_FALLTHROUGH.
(c_parser_label): Set FALLTHROUGH_LABEL_P on labels.  Handle
attribute fallthrough after a case label or default label.
(c_parser_statement_after_labels): Handle RID_ATTRIBUTE.
gcc/cp/
* constexpr.c (cxx_eval_internal_function): Handle IFN_FALLTHROUGH.
(potential_constant_expression_1): Likewise.
* constraint.cc (function_concept_check_p): Check fn for null.
* parser.c (cp_parser_primary_expression): Handle RID_ATTRIBUTE.
(cp_parser_statement): Handle fallthrough attribute.
(cp_parser_label_for_labeled_statement): Set FALLTHROUGH_LABEL_P on
labels.
(cp_parser_std_attribute): Handle fallthrough attribute.
(cp_parser_check_std_attribute): Detect duplicated fallthrough
attribute.
* pt.c (tsubst_copy_and_build): Handle internal functions.
(instantiation_dependent_scope_ref_p): Return if the expression is
null.
gcc/testsuite/
* c-c++-common/Wimplicit-fallthrough-1.c: New test.
* c-c++-common/Wimplicit-fallthrough-10.c: New test.
* c-c++-common/Wimplicit-fallthrough-11.c: New test.
* c-c++-common/Wimplicit-fallthrough-12.c: New test.
* c-c++-common/Wimplicit-fallthrough-13.c: New test.
* c-c++-common/Wimplicit-fallthrough-14.c: New test.
* c-c++-common/Wimplicit-fallthrough-15.c: New test.
* c-c++-common/Wimplicit-fallthrough-16.c: New test.
* c-c++-common/Wimplicit-fallthrough-17.c: New test.
* c-c++-common/Wimplicit-fallthrough-18.c: New test.
* c-c++-common/Wimplicit-fallthrough-19.c: New