Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Marek Polacek]

On Fri, Sep 30, 2016 at 10:05:57PM +0200, Jakub Jelinek wrote:
> On Fri, Sep 30, 2016 at 11:26:27AM +0200, Marek Polacek wrote:
> > I haven't gone over the patch in detail yet, but I wonder if we should
> > also accept /* Else, fall through.  */ (to be found e.g. in 
> > aarch64-simd.md).
> 
> Here is the patch split into a series of 3 patches (the later patches depend
> on the earlier ones):
> 1) the first patch fixes some bugs and fixes also -Wimplicit-fallthrough -C
> 2) the second patch adds the else and intentional/ly etc.
> 3) the third one adds the optional comma between else and fall
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok?
> 
>   Jakub

> 2016-09-30  Jakub Jelinek  
> 
>   * c-lex.c (c_lex_with_flags) : For CPP_COMMENT
>   token with PREV_FALLTHROUGH, skip all following CPP_PADDING and
>   CPP_COMMENT tokens and set add_flags to PREV_FALLTHROUGH afterwards.
> 
>   * doc/invoke.texi (-Wimplicit-fallthrough): Document the accepted
>   FALLTHRU comment styles.
> 
>   * lex.c (fallthrough_comment_p): Fix off-by-one size comparison
>   errors, cleanup.
>   (_cpp_lex_direct): Allow arbitrary comments in between
>   fallthrough_comment_p comment and following token.
> 
>   * c-c++-common/Wimplicit-fallthrough-22.c: New test.
>   * c-c++-common/Wimplicit-fallthrough-23.c: New test.
> 
> --- gcc/c-family/c-lex.c.jj   2016-09-30 18:16:26.303336781 +0200
> +++ gcc/c-family/c-lex.c  2016-09-30 18:26:14.650999215 +0200
> @@ -598,7 +598,18 @@ c_lex_with_flags (tree *value, location_
>  
>  /* CPP_COMMENT will appear when compiling with -C and should be
> ignored.  */
> - case CPP_COMMENT:
> +case CPP_COMMENT:
> +  if (tok->flags & PREV_FALLTHROUGH)
> + {
> +   do
> + {
> +   tok = cpp_get_token_with_location (parse_in, loc);
> +   type = tok->type;
> + }
> +   while (type == CPP_PADDING || type == CPP_COMMENT);
> +   add_flags = PREV_FALLTHROUGH;

Wouldn't |= be safer?

> +   goto retry_after_at;
> + }

And the comment is not really true anymore; I think let's say that we
want to set PREV_FALLTHROUGH on the token following the comment.

Otherwise I'm ok with the first patch, just please rename the 
-22 test to -24, since I've added -22.c and -23.c.

Marek

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Eric Botcazou]

> I think the vast majority of the comments I changed (removing "...")
> wouldn't have to be changed were this patch in.

So can we install it instead of arguing about hypothetical things?

-- 
Eric Botcazou

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Marek Polacek]

On Tue, Oct 04, 2016 at 05:58:17PM +0200, Michael Matz wrote:
> Hi,
> 
> On Sat, 1 Oct 2016, Jakub Jelinek wrote:
> 
> > > -  /* ... fall through for unsigned ints ...  */
> > > +  /* fall through */
> > > 
> > > -/* For other instructions, fallthru.  */
> > > +/* fallthru.  */
> > > 
> > > -  /* fall thru to manual case */
> > > +  /* fall thru */
> > > 
> > > 
> > > So, because of its excessive pickiness, the warning ends up making the 
> > > user 
> > > butcher informative comments.  How is that helpful?
> > 
> > Note, the wast majority of the fallthru comments are already recognized, it
> > is only when people start to write those in free form.
> 
> Which is what happens often.  How can you not see that with patches 
> transforming from free form to strict form fly by _on this very mailing 
> list_ ?

I think the vast majority of the comments I changed (removing "...") wouldn't
have to be changed were this patch in.  But it didn't hurt, either.

Marek

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Michael Matz]

Hi,

On Sat, 1 Oct 2016, Jakub Jelinek wrote:

> > -  /* ... fall through for unsigned ints ...  */
> > +  /* fall through */
> > 
> > -/* For other instructions, fallthru.  */
> > +/* fallthru.  */
> > 
> > -  /* fall thru to manual case */
> > +  /* fall thru */
> > 
> > 
> > So, because of its excessive pickiness, the warning ends up making the user 
> > butcher informative comments.  How is that helpful?
> 
> Note, the wast majority of the fallthru comments are already recognized, it
> is only when people start to write those in free form.

Which is what happens often.  How can you not see that with patches 
transforming from free form to strict form fly by _on this very mailing 
list_ ?

> E.g. today I wanted to try Marek's testcase from some PR and have commented
> out [[fallthrough]]; attribute - // [[fallthrough]];
> if we are not picky enough, it will be handled as a valid fallthrough
> comment, which might not be the intent.

That quickly gets into absurd arguments for strictness.


Ciao,
Michael.

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Tom Tromey]

> "Eric" == Eric Botcazou  writes:

Eric> So, because of its excessive pickiness, the warning ends up making the 
user 
Eric> butcher informative comments.  How is that helpful?

Those comments are not informative.  In most cases I kept the original
text just to forestall complaints.  But really if you read those
comments they are pointless.

That said, it would be better by far to have a mode where only the
attribute is accepted, and where comments aren't parsed.  Then gcc could
also warn when the attribute is used incorrectly.  The reason this is
preferable is that it helps protect against more errors, say those
introduced by merge mistakes.

Tom

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Jakub Jelinek]

On Sat, Oct 01, 2016 at 10:49:03AM +0200, Eric Botcazou wrote:
> > See Tom Tromey's explanation why accepting too much is bad (at least unless
> > we want multiple levels).
> 
> Tom's changes made to GDB are IMO the perfect examples of what we don't want:
> 
> -  /* ... fall through for unsigned ints ...  */
> +  /* fall through */
> 
> -/* For other instructions, fallthru.  */
> +/* fallthru.  */
> 
> -  /* fall thru to manual case */
> +  /* fall thru */
> 
> 
> So, because of its excessive pickiness, the warning ends up making the user 
> butcher informative comments.  How is that helpful?

Note, the wast majority of the fallthru comments are already recognized, it
is only when people start to write those in free form.
E.g. today I wanted to try Marek's testcase from some PR and have commented
out [[fallthrough]]; attribute - // [[fallthrough]];
if we are not picky enough, it will be handled as a valid fallthrough
comment, which might not be the intent.  The more this is discussed,
the more I lean towards the multiple levels of the warning, so that projects
can choose what exactly they want, starting with only allowing attributes,
down to accepting any comments whatsoever.

Jakub

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Eric Botcazou]

> See Tom Tromey's explanation why accepting too much is bad (at least unless
> we want multiple levels).

Tom's changes made to GDB are IMO the perfect examples of what we don't want:

-  /* ... fall through for unsigned ints ...  */
+  /* fall through */

-/* For other instructions, fallthru.  */
+/* fallthru.  */

-  /* fall thru to manual case */
+  /* fall thru */


So, because of its excessive pickiness, the warning ends up making the user 
butcher informative comments.  How is that helpful?

-- 
Eric Botcazou

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Jason Merrill]

On Fri, Sep 30, 2016 at 7:10 AM, Bernd Schmidt  wrote:
> On 09/30/2016 12:51 PM, Jakub Jelinek wrote:
>> On Fri, Sep 30, 2016 at 12:42:20PM +0200, Bernd Schmidt wrote:
>>> On 09/30/2016 11:45 AM, Jakub Jelinek wrote:
>>>
 See Tom Tromey's explanation why accepting too much is bad (at least
 unless we want multiple levels).
>>>
>>> And I still don't buy it. The case where someone writes "Don't fall
>>> through" is artificial to begin with, and also forgetting to put the break; 
>>> there
>>> really isn't something for us to be concerned about.

Agreed.

>> It doesn't have to be exactly that, fall is a substring of 200+ english,
>> thr is a substring of 1000+ english words, if you allow arbitrary stuff in
>> between or before it or after it, it can say anything, completely
>> unrelated to falling through across case labels.
>
> True, but IMO irrelevant. We have to consider what is likely in real code.
> We're trying to catch likely problems without adding a prohibitive cost to
> enabling -Wextra. It's not helpful to punish people for writing code that is
> valid and documents intentional fallthrough, but does it in a style or a
> language we didn't expect.
>
> A complete solution would require working AI; maybe someone from Google can
> contribute one. Failing that, we need heuristics, and I still like Michael's
> suggestion of not printing a warning if we see _any_ comment, on the grounds
> that this would still catch the vast majority of actual errors, without the
> huge false positive rate we currently have.

I think this would lead to too many false negatives, as explanatory
comments before a case label are extremely common.  I prefer the
*fall*thr{u,ough}* heuristic.

Jason

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Jakub Jelinek]

On Fri, Sep 30, 2016 at 11:26:27AM +0200, Marek Polacek wrote:
> I haven't gone over the patch in detail yet, but I wonder if we should
> also accept /* Else, fall through.  */ (to be found e.g. in aarch64-simd.md).

Here is the patch split into a series of 3 patches (the later patches depend
on the earlier ones):
1) the first patch fixes some bugs and fixes also -Wimplicit-fallthrough -C
2) the second patch adds the else and intentional/ly etc.
3) the third one adds the optional comma between else and fall

Bootstrapped/regtested on x86_64-linux and i686-linux, ok?

Jakub
2016-09-30  Jakub Jelinek  

* c-lex.c (c_lex_with_flags) : For CPP_COMMENT
token with PREV_FALLTHROUGH, skip all following CPP_PADDING and
CPP_COMMENT tokens and set add_flags to PREV_FALLTHROUGH afterwards.

* doc/invoke.texi (-Wimplicit-fallthrough): Document the accepted
FALLTHRU comment styles.

* lex.c (fallthrough_comment_p): Fix off-by-one size comparison
errors, cleanup.
(_cpp_lex_direct): Allow arbitrary comments in between
fallthrough_comment_p comment and following token.

* c-c++-common/Wimplicit-fallthrough-22.c: New test.
* c-c++-common/Wimplicit-fallthrough-23.c: New test.

--- gcc/c-family/c-lex.c.jj 2016-09-30 18:16:26.303336781 +0200
+++ gcc/c-family/c-lex.c2016-09-30 18:26:14.650999215 +0200
@@ -598,7 +598,18 @@ c_lex_with_flags (tree *value, location_
 
 /* CPP_COMMENT will appear when compiling with -C and should be
ignored.  */
- case CPP_COMMENT:
+case CPP_COMMENT:
+  if (tok->flags & PREV_FALLTHROUGH)
+   {
+ do
+   {
+ tok = cpp_get_token_with_location (parse_in, loc);
+ type = tok->type;
+   }
+ while (type == CPP_PADDING || type == CPP_COMMENT);
+ add_flags = PREV_FALLTHROUGH;
+ goto retry_after_at;
+   }
goto retry;
 
 default:
--- gcc/doc/invoke.texi.jj  2016-09-30 18:16:26.308336719 +0200
+++ gcc/doc/invoke.texi 2016-09-30 18:26:33.288766793 +0200
@@ -4157,10 +4157,26 @@ C++17 provides a standard way to suppres
 warning using @code{[[fallthrough]];} instead of the GNU attribute.  In C++11
 or C++14 users can use @code{[[gnu::fallthrough]];}, which is a GNU extension.
 Instead of the these attributes, it is also possible to add a "falls through"
-comment to silence the warning.  GCC accepts a wide range of such comments,
-for example all of "Falls through.", "fallthru", "FALLS-THROUGH" work.  This
-comment needs to consist of two words merely, optionally followed by periods
-or whitespaces.
+comment to silence the warning.  The whole body of the C or C++ style comment
+should match one of the following regular expressions:
+
+@itemize @bullet
+
+@item @code{-fallthrough}
+
+@item @code{@@fallthrough@@}
+
+@item @code{[ \t]*FALL(S | |-)?THR(OUGH|U)\.?[ \t]*}
+
+@item @code{[ \t]*Fall((s | |-)[Tt]|t)hr(ough|u)\.?[ \t]*}
+
+@item @code{[ \t]*fall(s | |-)?thr(ough|u)\.?[ \t]*}
+
+@end itemize
+
+and the comment needs to be followed after optional whitespace and other 
comments
+by @code{case} or @code{default} keywords or by a user label that preceeds some
+@code{case} or @code{default} label.
 
 @smallexample
 @group
--- libcpp/lex.c.jj 2016-09-30 18:16:22.987378137 +0200
+++ libcpp/lex.c2016-09-30 18:26:33.289766781 +0200
@@ -2060,7 +2060,7 @@ fallthrough_comment_p (cpp_reader *pfile
 }
   /* Whole comment contents (regex):
  [ \t]*FALL(S | |-)?THR(OUGH|U)\.?[ \t]*
- [ \t]*Fall(s | |-)?[Tt]hr(ough|u)\.?[ \t]*
+ [ \t]*Fall((s | |-)[Tt]|t)hr(ough|u)\.?[ \t]*
  [ \t]*fall(s | |-)?thr(ough|u)\.?[ \t]*
*/
   else
@@ -2070,30 +2070,27 @@ fallthrough_comment_p (cpp_reader *pfile
   unsigned char f = *from;
   if (f != 'F' && f != 'f')
return false;
-  if ((size_t) (pfile->buffer->cur - from) < sizeof "fallthrough")
+  if ((size_t) (pfile->buffer->cur - from) < sizeof "fallthru" - 1)
return false;
   bool all_upper = false;
   if (f == 'F' && memcmp (from + 1, "ALL", sizeof "ALL" - 1) == 0)
all_upper = true;
   else if (memcmp (from + 1, "all", sizeof "all" - 1))
return false;
-  if (from[sizeof "fall" - 1] == (all_upper ? 'S' : 's')
- && from[sizeof "falls" - 1] == ' ')
-   from += sizeof "falls " - 1;
-  else if (from[sizeof "fall" - 1] == ' '
-  || from[sizeof "fall" - 1] == '-')
-   from += sizeof "fall " - 1;
-  else if (from[sizeof "fall" - 1] != (all_upper ? 'T' : 't'))
+  from += sizeof "fall" - 1;
+  if (*from == (all_upper ? 'S' : 's') && from[1] == ' ')
+   from += 2;
+  else if (*from == ' ' || *from == '-')
+   from++;
+  else if (*from != (all_upper ? 'T' : 't'))
return false;
-  else
-   from += sizeof "fall" - 1;
   if ((f == 'f' || *from != 'T') && (all_upper || *from != 't'))
  

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Jakub Jelinek]

On Fri, Sep 30, 2016 at 10:10:55AM -0600, Martin Sebor wrote:
> I haven't been following the discussion very closely so I may have
> missed that what I'm about to suggest has been discussed and rejected
> for some valid reason, but if not let me try.
> 
> It seems to me that the ultimate, long term goal should be to have
> actively maintained code bases gradually migrate away from the
> various fallthrough comments and to the new attribute.  Under that
> premise, I think introducing a warning that's on the permissive end
> of the spectrum (say outside of -Wall, or even outside of -Wextra,
> and/or disabling the checker at the first sight of a comment) would
> obviate the concern of needlessly breaking working code and let
> users start adopting the warning on their own schedules.  The next
> major release of GCC after 7 could increase the sensitivity of the
> warning (e.g., by adding it -Wextra, and/or checking for the words
> fall though in the comments, etc.), and the next one could make it
> even more strict.  With GCC's one year release cycle this approach
> would give even users who adopt the latest compiler two to three
> years to make the transition.

That is IMHO a bad idea, because almost nobody will really use it and the
warning option will bitrot.  The reason why we've added parsing of the most
common comments was exactly to be able to enable it already in -Wextra.
clang has it outside of -Wextra and (almost) nobody really started adopting
the attributes.

The option to have different levels of -Wimplicit-fallthrough= has been
proposed, that would allow projects to choose if they only allow attributes
to disable the warning, or also some very strict set of comments, or more
relaxed set of comments, or perhaps any comment before the label.

Jakub

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Martin Sebor]

I haven't been following the discussion very closely so I may have
missed that what I'm about to suggest has been discussed and rejected
for some valid reason, but if not let me try.

It seems to me that the ultimate, long term goal should be to have
actively maintained code bases gradually migrate away from the
various fallthrough comments and to the new attribute.  Under that
premise, I think introducing a warning that's on the permissive end
of the spectrum (say outside of -Wall, or even outside of -Wextra,
and/or disabling the checker at the first sight of a comment) would
obviate the concern of needlessly breaking working code and let
users start adopting the warning on their own schedules.  The next
major release of GCC after 7 could increase the sensitivity of the
warning (e.g., by adding it -Wextra, and/or checking for the words
fall though in the comments, etc.), and the next one could make it
even more strict.  With GCC's one year release cycle this approach
would give even users who adopt the latest compiler two to three
years to make the transition.

Martin

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Marek Polacek]

On Fri, Sep 30, 2016 at 12:42:20PM +0200, Bernd Schmidt wrote:
> On 09/30/2016 11:45 AM, Jakub Jelinek wrote:
> 
> > See Tom Tromey's explanation why accepting too much is bad (at least unless
> > we want multiple levels).
> 
> And I still don't buy it. The case where someone writes "Don't fall through"
> is artificial to begin with, and also forgetting to put the break; there
> really isn't something for us to be concerned about.
> 
> On the other hand, we end up having to change massive amounts of perfectly
> fine code and even disabling -Werror in some places. That's the textbook
> case of an awful warning.

On the flip side, those patches (for GCC) were quite trivial and I've done
most of the work, and it's something you only have to do once.

Disabling -Werror was only necessary because I didn't investigate the problem
deep enough, but this has been fixed now (the genattr.c part in
) and I'm about to
post a patch that removes those added -Wno-error (just waiting for aarch64
bootstrap to finish).

The warning repeatedly finds bugs (I've already fixed a bunch of them), and
prevents from more creeping in.

The comments parsing part is contentious and tricky (and GCC might be the first
compiler that attempts to do this, so we're breaking new ground), yet the
warning *is* useful, and only more so with upcoming C++17 which standardizes
[[fallthrough]];.

Marek

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Bernd Schmidt]

On 09/30/2016 12:51 PM, Jakub Jelinek wrote:

On Fri, Sep 30, 2016 at 12:42:20PM +0200, Bernd Schmidt wrote:

On 09/30/2016 11:45 AM, Jakub Jelinek wrote:


See Tom Tromey's explanation why accepting too much is bad (at least unless
we want multiple levels).


And I still don't buy it. The case where someone writes "Don't fall through"
is artificial to begin with, and also forgetting to put the break; there
really isn't something for us to be concerned about.


It doesn't have to be exactly that, fall is a substring of 200+ english, thr
is a substring of 1000+ english words, if you allow arbitrary stuff in
between or before it or after it, it can say anything, completely unrelated
to falling through across case labels.


True, but IMO irrelevant. We have to consider what is likely in real 
code. We're trying to catch likely problems without adding a prohibitive 
cost to enabling -Wextra. It's not helpful to punish people for writing 
code that is valid and documents intentional fallthrough, but does it in 
a style or a language we didn't expect.


A complete solution would require working AI; maybe someone from Google 
can contribute one. Failing that, we need heuristics, and I still like 
Michael's suggestion of not printing a warning if we see _any_ comment, 
on the grounds that this would still catch the vast majority of actual 
errors, without the huge false positive rate we currently have.



Bernd

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Jakub Jelinek]

On Fri, Sep 30, 2016 at 12:42:20PM +0200, Bernd Schmidt wrote:
> On 09/30/2016 11:45 AM, Jakub Jelinek wrote:
> 
> >See Tom Tromey's explanation why accepting too much is bad (at least unless
> >we want multiple levels).
> 
> And I still don't buy it. The case where someone writes "Don't fall through"
> is artificial to begin with, and also forgetting to put the break; there
> really isn't something for us to be concerned about.

It doesn't have to be exactly that, fall is a substring of 200+ english, thr
is a substring of 1000+ english words, if you allow arbitrary stuff in
between or before it or after it, it can say anything, completely unrelated
to falling through across case labels.

> On the other hand, we end up having to change massive amounts of perfectly
> fine code and even disabling -Werror in some places. That's the textbook
> case of an awful warning.

First of all, the warning is only in -Wextra, so will affect minority of
programs that use it, and is likely many other a style warning, it is a good
thing to enforce consistent style of these comments or eventually the
attributes for projects that care about -Wextra, and as a bonus they will be
able to fix real bugs in their projects.  Note that lint is considerably
more strict in what it accepts.

Jakub

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Bernd Schmidt]

On 09/30/2016 11:45 AM, Jakub Jelinek wrote:


See Tom Tromey's explanation why accepting too much is bad (at least unless
we want multiple levels).


And I still don't buy it. The case where someone writes "Don't fall 
through" is artificial to begin with, and also forgetting to put the 
break; there really isn't something for us to be concerned about.


On the other hand, we end up having to change massive amounts of 
perfectly fine code and even disabling -Werror in some places. That's 
the textbook case of an awful warning.



Bernd

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Marek Polacek]

On Fri, Sep 30, 2016 at 11:31:43AM +0200, Jakub Jelinek wrote:
> On Fri, Sep 30, 2016 at 11:26:27AM +0200, Marek Polacek wrote:
> > On Thu, Sep 29, 2016 at 10:16:33PM +0200, Jakub Jelinek wrote:
> > > The following patch does a few things:
> > > 1) fixes -Wimplicit-fallthrough -C
> > >(with -C the PREV_FALLTHROUGH flag is on the CPP_COMMENT token, we need
> > > to propagate it to the C/C++ token's flags in the FEs)
> > > 2) it accepts a comment in between /* FALLTHRU */ comment and the
> > >case/default keyword or user label, people often write:
> > >  ...
> > >  /* FALLTHRU */
> > > 
> > >  /* Rationale or description of what the following code does.  */
> > >  case ...:
> > >and forcing users to move their comments after the labels or after the
> > >first label might be too annoying
> > > 3) it adds support for some common FALLTHRU comment styles that appeared
> > >in GCC sources, or in Linux kernel etc., e.g.:
> > > 
> > >/*lint -fallthrough */
> > > 
> > >/* ... falls through ... */
> > > 
> > >/* else fall-through */
> > > 
> > >/* Intentional fall through.  */
> > > 
> > >/* FALLTHRU - some explanation why.  */
> > 
> > I haven't gone over the patch in detail yet, but I wonder if we should
> > also accept /* Else, fall through.  */ (to be found e.g. in 
> > aarch64-simd.md).
> 
> Clearly people are extremely creative with these comments, maybe it would be
> better to just remove the new additions from the patch I've posted (drop the
> else/intentational/intentationally/... around/!!! around etc., to force
> people to standardize on something), and just apply the fixes and support
> for comments in between.

Obviously you can get a very wide range of opinions here.  I like the patch;
while I don't think we should allow complete free form, accepting stuff like
/* ... falls through ... */
or
/* else fall-through */
is a good thing.

Marek

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Jakub Jelinek]

On Fri, Sep 30, 2016 at 11:42:12AM +0200, Eric Botcazou wrote:
> > Clearly people are extremely creative with these comments, maybe it would be
> > better to just remove the new additions from the patch I've posted (drop
> > the else/intentational/intentationally/... around/!!! around etc., to force
> > people to standardize on something), and just apply the fixes and support
> > for comments in between.
> 
> Maybe just match "fall" and "through/thru/etc" positively and "not/n't/etc" 
> negatively on the same line.

See Tom Tromey's explanation why accepting too much is bad (at least unless
we want multiple levels).

Jakub

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Eric Botcazou]

> Clearly people are extremely creative with these comments, maybe it would be
> better to just remove the new additions from the patch I've posted (drop
> the else/intentational/intentationally/... around/!!! around etc., to force
> people to standardize on something), and just apply the fixes and support
> for comments in between.

Maybe just match "fall" and "through/thru/etc" positively and "not/n't/etc" 
negatively on the same line.

-- 
Eric Botcazou

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Jakub Jelinek]

On Fri, Sep 30, 2016 at 11:26:27AM +0200, Marek Polacek wrote:
> On Thu, Sep 29, 2016 at 10:16:33PM +0200, Jakub Jelinek wrote:
> > The following patch does a few things:
> > 1) fixes -Wimplicit-fallthrough -C
> >(with -C the PREV_FALLTHROUGH flag is on the CPP_COMMENT token, we need
> > to propagate it to the C/C++ token's flags in the FEs)
> > 2) it accepts a comment in between /* FALLTHRU */ comment and the
> >case/default keyword or user label, people often write:
> >  ...
> >  /* FALLTHRU */
> > 
> >  /* Rationale or description of what the following code does.  */
> >  case ...:
> >and forcing users to move their comments after the labels or after the
> >first label might be too annoying
> > 3) it adds support for some common FALLTHRU comment styles that appeared
> >in GCC sources, or in Linux kernel etc., e.g.:
> > 
> >/*lint -fallthrough */
> > 
> >/* ... falls through ... */
> > 
> >/* else fall-through */
> > 
> >/* Intentional fall through.  */
> > 
> >/* FALLTHRU - some explanation why.  */
> 
> I haven't gone over the patch in detail yet, but I wonder if we should
> also accept /* Else, fall through.  */ (to be found e.g. in aarch64-simd.md).

Clearly people are extremely creative with these comments, maybe it would be
better to just remove the new additions from the patch I've posted (drop the
else/intentational/intentationally/... around/!!! around etc., to force
people to standardize on something), and just apply the fixes and support
for comments in between.

Jakub

Re: [PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Marek Polacek]

On Thu, Sep 29, 2016 at 10:16:33PM +0200, Jakub Jelinek wrote:
> Hi!
> 
> The following patch does a few things:
> 1) fixes -Wimplicit-fallthrough -C
>(with -C the PREV_FALLTHROUGH flag is on the CPP_COMMENT token, we need
> to propagate it to the C/C++ token's flags in the FEs)
> 2) it accepts a comment in between /* FALLTHRU */ comment and the
>case/default keyword or user label, people often write:
>  ...
>  /* FALLTHRU */
> 
>  /* Rationale or description of what the following code does.  */
>  case ...:
>and forcing users to move their comments after the labels or after the
>first label might be too annoying
> 3) it adds support for some common FALLTHRU comment styles that appeared
>in GCC sources, or in Linux kernel etc., e.g.:
> 
>/*lint -fallthrough */
> 
>/* ... falls through ... */
> 
>/* else fall-through */
> 
>/* Intentional fall through.  */
> 
>/* FALLTHRU - some explanation why.  */

I haven't gone over the patch in detail yet, but I wonder if we should
also accept /* Else, fall through.  */ (to be found e.g. in aarch64-simd.md).

Marek

[PATCH] Fix -Wimplicit-fallthrough -C, handle some more comment styles and comments in between FALLTHRU comment and label

[Jakub Jelinek]

Hi!

The following patch does a few things:
1) fixes -Wimplicit-fallthrough -C
   (with -C the PREV_FALLTHROUGH flag is on the CPP_COMMENT token, we need
to propagate it to the C/C++ token's flags in the FEs)
2) it accepts a comment in between /* FALLTHRU */ comment and the
   case/default keyword or user label, people often write:
 ...
 /* FALLTHRU */

 /* Rationale or description of what the following code does.  */
 case ...:
   and forcing users to move their comments after the labels or after the
   first label might be too annoying
3) it adds support for some common FALLTHRU comment styles that appeared
   in GCC sources, or in Linux kernel etc., e.g.:

   /*lint -fallthrough */

   /* ... falls through ... */

   /* else fall-through */

   /* Intentional fall through.  */

   /* FALLTHRU - some explanation why.  */
4) it attempts to precisely document what is supported in the documentation
5) some libcpp fixes in the fallthrough_comment_p routine, in particular
   some even previously documented coment styles could be rejected in C++
   style comments if there wasn't any whitespace after them
6) testcase covering various forms

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

2016-09-29  Jakub Jelinek  

* c-lex.c (c_lex_with_flags) : For CPP_COMMENT
token with PREV_FALLTHROUGH, skip all following CPP_PADDING and
CPP_COMMENT tokens and set add_flags to PREV_FALLTHROUGH afterwards.

* doc/invoke.texi (-Wimplicit-fallthrough): Document the accepted
FALLTHRU comment styles.

* lex.c (fallthrough_comment_p): Extend to handle more common FALLTHRU
comment styles.
(_cpp_lex_direct): Allow arbitrary comments in between
fallthrough_comment_p comment and following token.

* c-c++-common/Wimplicit-fallthrough-22.c: New test.
* c-c++-common/Wimplicit-fallthrough-23.c: New test.

--- gcc/c-family/c-lex.c.jj 2016-09-27 09:46:07.0 +0200
+++ gcc/c-family/c-lex.c2016-09-29 12:11:30.633532650 +0200
@@ -598,7 +598,18 @@ c_lex_with_flags (tree *value, location_
 
 /* CPP_COMMENT will appear when compiling with -C and should be
ignored.  */
- case CPP_COMMENT:
+case CPP_COMMENT:
+  if (tok->flags & PREV_FALLTHROUGH)
+   {
+ do
+   {
+ tok = cpp_get_token_with_location (parse_in, loc);
+ type = tok->type;
+   }
+ while (type == CPP_PADDING || type == CPP_COMMENT);
+ add_flags = PREV_FALLTHROUGH;
+ goto retry_after_at;
+   }
goto retry;
 
 default:
--- gcc/doc/invoke.texi.jj  2016-09-27 09:46:07.0 +0200
+++ gcc/doc/invoke.texi 2016-09-29 13:19:41.046697347 +0200
@@ -4156,10 +4156,28 @@ C++17 provides a standard way to suppres
 warning using @code{[[fallthrough]];} instead of the GNU attribute.  In C++11
 or C++14 users can use @code{[[gnu::fallthrough]];}, which is a GNU extension.
 Instead of the these attributes, it is also possible to add a "falls through"
-comment to silence the warning.  GCC accepts a wide range of such comments,
-for example all of "Falls through.", "fallthru", "FALLS-THROUGH" work.  This
-comment needs to consist of two words merely, optionally followed by periods
-or whitespaces.
+comment to silence the warning.  The whole body of the C or C++ style comment
+should match one of the following regular expressions:
+
+@itemize @bullet
+
+@item @code{-fallthrough}
+
+@item @code{@@fallthrough@@}
+
+@item @code{lint -fallthrough ?}
+
+@item @code{[ \t.!]*(ELSE |INTENTIONAL(LY)? )?FALL(S | |-)?THR(OUGH|U)[ 
\t.!]*(-[^\n\r]*)?}
+
+@item @code{[ \t.!]*(Else |Intentional(ly)? )?Fall((s | |-)[Tt]|t)hr(ough|u)[ 
\t.!]*(-[^\n\r]*)?}
+
+@item @code{[ \t.!]*([Ee]lse |[Ii]ntentional(ly)? )?fall(s | |-)?thr(ough|u)[ 
\t.!]*(-[^\n\r]*)?}
+
+@end itemize
+
+and the comment needs to be followed after optional whitespace and other 
comments
+by @code{case} or @code{default} keywords or by a user label that preceeds some
+@code{case} or @code{default} label.
 
 @smallexample
 @group
--- libcpp/lex.c.jj 2016-09-26 12:06:49.0 +0200
+++ libcpp/lex.c2016-09-29 13:54:12.703757398 +0200
@@ -2059,41 +2059,102 @@ fallthrough_comment_p (cpp_reader *pfile
   from += 1 + len;
 }
   /* Whole comment contents (regex):
- [ \t]*FALL(S | |-)?THR(OUGH|U)\.?[ \t]*
- [ \t]*Fall(s | |-)?[Tt]hr(ough|u)\.?[ \t]*
- [ \t]*fall(s | |-)?thr(ough|u)\.?[ \t]*
+ lint -fallthrough ?
+   */
+  else if (*from == 'l')
+{
+  size_t len = sizeof "int -fallthrough" - 1;
+  if ((size_t) (pfile->buffer->cur - from - 1) < len)
+   return false;
+  if (memcmp (from + 1, "int -fallthrough", len))
+return false;
+  from += 1 + len;
+  if (*from == ' ')
+from++;
+}
+  /* Whole comment contents (regex):
+ [ \t.!]*(ELSE |INTENTIONAL(LY)? )?FALL(S | |-)?THR(OUGH|U)[