Re: [PATCH] C: detect more missing semicolons (PR c/7356)

2017-10-25 Thread David Malcolm 
On Wed, 2017-10-25 at 09:59 -0600, Jeff Law wrote:
> On 10/11/2017 01:32 PM, David Malcolm wrote:
> > [This patch assumes the two patches here:
> >  https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00274.html ]
> 
> I see the patch directly referenced in that message on the
> trunk.  But
> I'm not sure what you mean by "two patches".  If there's a prereq
> that
> hasn't been approved, let me know.

Sorry about the confusion; the one at the URL above was:

"[PATCH 2/2] C/C++: add fix-it hints for various missing symbols (v3)",

which is an updated version of:

"[PATCH 2/2] C/C++: add fix-it hints for various missing symbols (v2)"
https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01746.html

the other one I was referring to was:

"[PATCH 1/2] C++: avoid partial duplicate implementation of
cp_parser_error"
https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01745.html


Both these prereqs are in trunk (as r253686 and r253690 respectively).

> 
> > 
> > c_parser_declaration_or_fndef has logic for parsing what might be
> > either a declaration or a function definition.
> > 
> > This patch adds a test to detect cases where a semicolon would have
> > terminated the decls as a declaration, where the token that follows
> > would start a new declaration specifier, and updates the error
> > message
> > accordingly, with a fix-it hint.
> > 
> > This addresses PR c/7356, fixing the case of a stray token before a
> > #include which previously gave inscrutable output, and improving
> > e.g.:
> > 
> >   int i
> >   int j;
> > 
> > from:
> > 
> >   t.c:2:1: error: expected '=', ',', ';', 'asm' or '__attribute__'
> > before 'int'
> >int j;
> >^~~
> > 
> > to:
> > 
> >   t.c:1:6: error: expected ';' before 'int'
> >int i
> > ^
> > ;
> >int j;
> >~~~
> > 
> > The patch also adds a test for PR c/44515 as a baseline.
> 
> Personally I find the current error easier to digest.  It quite
> clearly
> tells me to look before the current token and shove in an appropriate
> thingie :-)   The extra vertical space, to me, makes the error
> message
> harder to parse.

You've been using gcc for multiple decades and are used to the existing
behavior, though ;)

> But I can see how others would prefer to see the point where the
> punctuation was missing.  So I won't let my biases get in the way
> here.

FWIW the benefit is a *lot* clearer for the first case described, where
the two locations are in different source files due to the missing
semicolon being immediately before a #include (PR c/7356), or at the
end of a header; it replaces dozens of crazy-looking errors with a
single sane one.

> > gcc/c/ChangeLog:
> > PR c/7356
> > * c-parser.c (c_parser_declaration_or_fndef): Detect missing
> > semicolons.
> > 
> > gcc/testsuite/ChangeLog:
> > PR c/7356
> > PR c/44515
> > * c-c++-common/pr44515.c: New test case.
> > * gcc.dg/pr7356-2.c: New test case.
> > * gcc.dg/pr7356.c: New test case.
> > * gcc.dg/spellcheck-typenames.c: Update the "singed" char
> > "TODO"
> > case to reflect changes to output.
> > * gcc.dg/noncompile/920923-1.c: Add dg-warning to reflect
> > changes
> > to output.
> 
> OK.

Thanks; committed to trunk as r254093 (and closing out PR c/7356 after
15 years!)

Dave

Re: [PATCH] C: detect more missing semicolons (PR c/7356)

2017-10-25 Thread Jeff Law 
On 10/11/2017 01:32 PM, David Malcolm wrote:
> [This patch assumes the two patches here:
>  https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00274.html ]
I see the patch directly referenced in that message on the trunk.  But
I'm not sure what you mean by "two patches".  If there's a prereq that
hasn't been approved, let me know.


> 
> c_parser_declaration_or_fndef has logic for parsing what might be
> either a declaration or a function definition.
> 
> This patch adds a test to detect cases where a semicolon would have
> terminated the decls as a declaration, where the token that follows
> would start a new declaration specifier, and updates the error message
> accordingly, with a fix-it hint.
> 
> This addresses PR c/7356, fixing the case of a stray token before a
> #include which previously gave inscrutable output, and improving e.g.:
> 
>   int i
>   int j;
> 
> from:
> 
>   t.c:2:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before 
> 'int'
>int j;
>^~~
> 
> to:
> 
>   t.c:1:6: error: expected ';' before 'int'
>int i
> ^
> ;
>int j;
>~~~
> 
> The patch also adds a test for PR c/44515 as a baseline.
Personally I find the current error easier to digest.  It quite clearly
tells me to look before the current token and shove in an appropriate
thingie :-)   The extra vertical space, to me, makes the error message
harder to parse.

But I can see how others would prefer to see the point where the
punctuation was missing.  So I won't let my biases get in the way here.



> 
> gcc/c/ChangeLog:
>   PR c/7356
>   * c-parser.c (c_parser_declaration_or_fndef): Detect missing
>   semicolons.
> 
> gcc/testsuite/ChangeLog:
>   PR c/7356
>   PR c/44515
>   * c-c++-common/pr44515.c: New test case.
>   * gcc.dg/pr7356-2.c: New test case.
>   * gcc.dg/pr7356.c: New test case.
>   * gcc.dg/spellcheck-typenames.c: Update the "singed" char "TODO"
>   case to reflect changes to output.
>   * gcc.dg/noncompile/920923-1.c: Add dg-warning to reflect changes
>   to output.
OK.
jeff

[PATCH] C: detect more missing semicolons (PR c/7356)

2017-10-11 Thread David Malcolm 
[This patch assumes the two patches here:
 https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00274.html ]

c_parser_declaration_or_fndef has logic for parsing what might be
either a declaration or a function definition.

This patch adds a test to detect cases where a semicolon would have
terminated the decls as a declaration, where the token that follows
would start a new declaration specifier, and updates the error message
accordingly, with a fix-it hint.

This addresses PR c/7356, fixing the case of a stray token before a
#include which previously gave inscrutable output, and improving e.g.:

  int i
  int j;

from:

  t.c:2:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'int'
   int j;
   ^~~

to:

  t.c:1:6: error: expected ';' before 'int'
   int i
^
;
   int j;
   ~~~

The patch also adds a test for PR c/44515 as a baseline.

gcc.dg/noncompile/920923-1.c needs a slight update, as the output for
the first line changes from:

  920923-1.c:2:14: error: expected '=', ',', ';', 'asm' or
  '__attribute__' before 'unsigned'
   typedef BYTE unsigned char; /* { dg-error "expected" } */
^~~~

to:
  920923-1.c:2:13: error: expected ';' before 'unsigned'
   typedef BYTE unsigned char; /* { dg-error "expected" } */
   ^
   ;
  920923-1.c:2:1: warning: useless type name in empty declaration
   typedef BYTE unsigned char; /* { dg-error "expected" } */
   ^~~

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
OK for trunk?

Thanks
Dave

gcc/c/ChangeLog:
PR c/7356
* c-parser.c (c_parser_declaration_or_fndef): Detect missing
semicolons.

gcc/testsuite/ChangeLog:
PR c/7356
PR c/44515
* c-c++-common/pr44515.c: New test case.
* gcc.dg/pr7356-2.c: New test case.
* gcc.dg/pr7356.c: New test case.
* gcc.dg/spellcheck-typenames.c: Update the "singed" char "TODO"
case to reflect changes to output.
* gcc.dg/noncompile/920923-1.c: Add dg-warning to reflect changes
to output.
---
 gcc/c/c-parser.c| 36 +
 gcc/testsuite/c-c++-common/pr44515.c| 14 +++
 gcc/testsuite/gcc.dg/noncompile/920923-1.c  |  1 +
 gcc/testsuite/gcc.dg/pr7356-2.c | 33 ++
 gcc/testsuite/gcc.dg/pr7356.c   | 17 ++
 gcc/testsuite/gcc.dg/spellcheck-typenames.c |  5 ++--
 6 files changed, 99 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/pr44515.c
 create mode 100644 gcc/testsuite/gcc.dg/pr7356-2.c
 create mode 100644 gcc/testsuite/gcc.dg/pr7356.c

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index a5e3ec4..7c3b834 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -2241,11 +2241,37 @@ c_parser_declaration_or_fndef (c_parser *parser, bool 
fndef_ok,
}
   if (!start_function (specs, declarator, all_prefix_attrs))
{
- /* This can appear in many cases looking nothing like a
-function definition, so we don't give a more specific
-error suggesting there was one.  */
- c_parser_error (parser, "expected %<=%>, %<,%>, %<;%>, % "
- "or %<__attribute__%>");
+ /* At this point we've consumed:
+  declaration-specifiers declarator
+and the next token isn't CPP_EQ, CPP_COMMA, CPP_SEMICOLON,
+RID_ASM, RID_ATTRIBUTE, or RID_IN,
+but the
+  declaration-specifiers declarator
+aren't grokkable as a function definition, so we have
+an error.  */
+ gcc_assert (!c_parser_next_token_is (parser, CPP_SEMICOLON));
+ if (c_parser_next_token_starts_declspecs (parser))
+   {
+ /* If we have
+  declaration-specifiers declarator decl-specs
+then assume we have a missing semicolon, which would
+give us:
+  declaration-specifiers declarator  decl-specs
+   ^
+   ;
+  <~ declaration ~~>
+Use c_parser_require to get an error with a fix-it hint.  */
+ c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>");
+ parser->error = false;
+   }
+ else
+   {
+ /* This can appear in many cases looking nothing like a
+function definition, so we don't give a more specific
+error suggesting there was one.  */
+ c_parser_error (parser, "expected %<=%>, %<,%>, %<;%>, % "
+ "or %<__attribute__%>");
+   }
  if (nested)
c_pop_function_context ();
  break;
diff --git a/gcc/testsuite/c-c++-common/pr44515.c 
b/gcc/testsuite/c-c++-common/pr44515.c
new file mode 100644