Re: Improve syntax error

2019-01-05 Thread Segher Boessenkool
Hi Daniel,

Some mostly boring comments:

On Fri, Jan 04, 2019 at 09:25:10PM +0100, Daniel Marjamäki wrote:
> The first reason is the hard problem, but maybe we can ignore this now also:
> 
> void f()
> {
> }  // <- looking at the indentation, it seems preferable to warn about 
> this
> }

I think the indentation warnings should catch that?

> diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> index 972b629c092..eabc5ffa15e 100644
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -171,6 +171,8 @@ struct GTY(()) c_parser {
>/* How many look-ahead tokens are available (0 - 4, or
>   more if parsing from pre-lexed tokens).  */
>unsigned int tokens_avail;
> +  /* nesting depth in expression (parentheses / squares) */

Start sentences with a capital, and end with full stop space space.  I
realise this isn't a full sentence, but the comment right above does this
as well ;-)

> @@ -763,6 +765,22 @@ c_parser_next_tokens_start_declaration (c_parser *parser)
>return false;
>  }
> 
> +/* Nesting start token */
> +
> +static bool c_parser_is_nesting_start (c_parser *parser)
> +{
> +return c_parser_next_token_is (parser, CPP_OPEN_PAREN) ||
> +   c_parser_next_token_is (parser, CPP_OPEN_SQUARE);

Indents should use tabs for every leading eight spaces.

> @@ -2228,7 +2264,10 @@ c_parser_declaration_or_fndef (c_parser
> *parser, bool fndef_ok,
>  }
>else
>  {
> -  c_parser_error (parser, "expected %<,%> or %<;%>");
> +  if (c_parser_unmatched_p (parser))
> +complain_about_unmatched_token (parser);

Should this say something like "expected ) or , or ;"?

> +  else
> +c_parser_error (parser, "expected %<,%> or %<;%>");
>c_parser_skip_to_end_of_block_or_statement (parser);
>return;
>  }


Segher


Re: Improve syntax error

2019-01-05 Thread Daniel Marjamäki
Thanks!

I will take care of the indentation and fix the comment.

> I think the indentation warnings should catch that?

I get this:

void f()
{
  }
} // <- error: expected identifier or '(' before '}' token

I ran with -Wall -Wextra -pedantic and did not see a indentation
warning. Am I missing some indentation warning? The error message I
get is a little misplaced. I think it's fine to warn about that } but
it could also say in the error message that the problem is probably
the previous }

> Should this say something like "expected ) or , or ;"?

No none of those suggestions will solve the error.

Look at this code:

int x = 3) + 0;

Writing a ) or , or ; will not fix the syntax error. You have to
remove the ) or add a ( somewhere.



Den lör 5 jan. 2019 kl 09:50 skrev Segher Boessenkool
:
>
> Hi Daniel,
>
> Some mostly boring comments:
>
> On Fri, Jan 04, 2019 at 09:25:10PM +0100, Daniel Marjamäki wrote:
> > The first reason is the hard problem, but maybe we can ignore this now also:
> >
> > void f()
> > {
> > }  // <- looking at the indentation, it seems preferable to warn about 
> > this
> > }
>
> I think the indentation warnings should catch that?
>
> > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> > index 972b629c092..eabc5ffa15e 100644
> > --- a/gcc/c/c-parser.c
> > +++ b/gcc/c/c-parser.c
> > @@ -171,6 +171,8 @@ struct GTY(()) c_parser {
> >/* How many look-ahead tokens are available (0 - 4, or
> >   more if parsing from pre-lexed tokens).  */
> >unsigned int tokens_avail;
> > +  /* nesting depth in expression (parentheses / squares) */
>
> Start sentences with a capital, and end with full stop space space.  I
> realise this isn't a full sentence, but the comment right above does this
> as well ;-)
>
> > @@ -763,6 +765,22 @@ c_parser_next_tokens_start_declaration (c_parser 
> > *parser)
> >return false;
> >  }
> >
> > +/* Nesting start token */
> > +
> > +static bool c_parser_is_nesting_start (c_parser *parser)
> > +{
> > +return c_parser_next_token_is (parser, CPP_OPEN_PAREN) ||
> > +   c_parser_next_token_is (parser, CPP_OPEN_SQUARE);
>
> Indents should use tabs for every leading eight spaces.
>
> > @@ -2228,7 +2264,10 @@ c_parser_declaration_or_fndef (c_parser
> > *parser, bool fndef_ok,
> >  }
> >else
> >  {
> > -  c_parser_error (parser, "expected %<,%> or %<;%>");
> > +  if (c_parser_unmatched_p (parser))
> > +complain_about_unmatched_token (parser);
>
> Should this say something like "expected ) or , or ;"?
>
> > +  else
> > +c_parser_error (parser, "expected %<,%> or %<;%>");
> >c_parser_skip_to_end_of_block_or_statement (parser);
> >return;
> >  }
>
>
> Segher


Re: Improve syntax error

2019-01-05 Thread Daniel Marjamäki
Here is a new patch with fixed comments and indentation

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 972b629c092..294ff34fe55 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -171,6 +171,8 @@ struct GTY(()) c_parser {
   /* How many look-ahead tokens are available (0 - 4, or
  more if parsing from pre-lexed tokens).  */
   unsigned int tokens_avail;
+  /* Nesting depth in expression (parentheses / squares). */
+  unsigned int nesting_depth;
   /* True if a syntax error is being recovered from; false otherwise.
  c_parser_error sets this flag.  It should clear this flag when
  enough tokens have been consumed to recover from the error.  */
@@ -763,6 +765,22 @@ c_parser_next_tokens_start_declaration (c_parser *parser)
   return false;
 }

+/* Nesting start token. */
+
+static bool c_parser_is_nesting_start (c_parser *parser)
+{
+  return c_parser_next_token_is (parser, CPP_OPEN_PAREN) ||
+ c_parser_next_token_is (parser, CPP_OPEN_SQUARE);
+}
+
+/* Nesting end token. */
+
+static bool c_parser_is_nesting_end (c_parser *parser)
+{
+  return c_parser_next_token_is (parser, CPP_CLOSE_PAREN) ||
+ c_parser_next_token_is (parser, CPP_CLOSE_SQUARE);
+}
+
 /* Consume the next token from PARSER.  */

 void
@@ -772,6 +790,10 @@ c_parser_consume_token (c_parser *parser)
   gcc_assert (parser->tokens[0].type != CPP_EOF);
   gcc_assert (!parser->in_pragma || parser->tokens[0].type != CPP_PRAGMA_EOL);
   gcc_assert (parser->error || parser->tokens[0].type != CPP_PRAGMA);
+  if (c_parser_is_nesting_start (parser))
+parser->nesting_depth++;
+  else if (parser->nesting_depth > 0 && c_parser_is_nesting_end (parser))
+parser->nesting_depth--;
   parser->last_token_location = parser->tokens[0].location;
   if (parser->tokens != &parser->tokens_buf[0])
 parser->tokens++;
@@ -1673,6 +1695,20 @@ add_debug_begin_stmt (location_t loc)
   add_stmt (stmt);
 }

+static bool c_parser_unmatched_p (c_parser *parser)
+{
+  if (c_parser_is_nesting_end (parser))
+return parser->nesting_depth == 0;
+  return false;
+}
+
+static void complain_about_unmatched_token (c_parser *parser)
+{
+  c_token *token = c_parser_peek_token (parser);
+  error_at (token->location, "unmatched %qs",
+cpp_type2name (token->type, token->flags));
+}
+
 /* Parse a declaration or function definition (C90 6.5, 6.7.1, C99
6.7, 6.9.1, C11 6.7, 6.9.1).  If FNDEF_OK is true, a function definition
is accepted; otherwise (old-style parameter declarations) only other
@@ -2228,7 +2264,10 @@ c_parser_declaration_or_fndef (c_parser
*parser, bool fndef_ok,
 }
   else
 {
-  c_parser_error (parser, "expected %<,%> or %<;%>");
+  if (c_parser_unmatched_p (parser))
+complain_about_unmatched_token (parser);
+  else
+c_parser_error (parser, "expected %<,%> or %<;%>");
   c_parser_skip_to_end_of_block_or_statement (parser);
   return;
 }
diff --git a/gcc/testsuite/gcc.dg/unmatched.c b/gcc/testsuite/gcc.dg/unmatched.c
new file mode 100644
index 000..bd458a01109
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/unmatched.c
@@ -0,0 +1,19 @@
+
+/* { dg-do compile } */
+
+void f1() {
+  int a = 0)+3; /* { dg-error "unmatched" } */
+}
+
+void f2() {
+  int b = (1]+3; /* { dg-error "expected" } */
+}
+
+void f3() {
+  int b = 1]+3; /* { dg-error "unmatched" } */
+}
+
+void f4() {
+  int c = (1))+3; /* { dg-error "unmatched" } */
+}
+

Den lör 5 jan. 2019 kl 18:02 skrev Daniel Marjamäki
:
>
> Thanks!
>
> I will take care of the indentation and fix the comment.
>
> > I think the indentation warnings should catch that?
>
> I get this:
>
> void f()
> {
>   }
> } // <- error: expected identifier or '(' before '}' token
>
> I ran with -Wall -Wextra -pedantic and did not see a indentation
> warning. Am I missing some indentation warning? The error message I
> get is a little misplaced. I think it's fine to warn about that } but
> it could also say in the error message that the problem is probably
> the previous }
>
> > Should this say something like "expected ) or , or ;"?
>
> No none of those suggestions will solve the error.
>
> Look at this code:
>
> int x = 3) + 0;
>
> Writing a ) or , or ; will not fix the syntax error. You have to
> remove the ) or add a ( somewhere.
>
>
>
> Den lör 5 jan. 2019 kl 09:50 skrev Segher Boessenkool
> :
> >
> > Hi Daniel,
> >
> > Some mostly boring comments:
> >
> > On Fri, Jan 04, 2019 at 09:25:10PM +0100, Daniel Marjamäki wrote:
> > > The first reason is the hard problem, but maybe we can ignore this now 
> > > also:
> > >
> > > void f()
> > > {
> > > }  // <- looking at the indentation, it seems preferable to warn 
> > > about this
> > > }
> >
> > I think the indentation warnings should catch that?
> >
> > > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> > > index 972b629c092..eabc5ffa15e 100644
> > > --- a/gcc/c/c-parser.c
> > > +++ b/gcc/c/c-parser.c
> > > @@ -171,6 +171,8 @@ struct