PING: Re: [PATCH 1/2] C++: avoid partial duplicate implementation of cp_parser_error

[David Malcolm <dmalcolm@redhat.com>]

Ping

On Tue, 2017-09-26 at 09:56 -0400, David Malcolm wrote:
> In r251026 (aka 3fe34694f0990d1d649711ede0326497f8a849dc,
> "C/C++: show pertinent open token when missing a close token")
> I copied part of cp_parser_error into cp_parser_required_error,
> leading to duplication of code.
> 
> This patch eliminates this duplication by merging the two copies of
> the
> code into a new cp_parser_error_1 subroutine.
> 
> Doing so removes an indentation level, making the patch appear to
> have
> more churn than it really does.
> 
> The patch also undoes the change to g++.dg/parse/pragma2.C, as the
> old behavior is restored.
> 
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> 
> OK for trunk?
> 
> gcc/cp/ChangeLog:
> 	* parser.c (get_matching_symbol): Move to before...
> 	(cp_parser_error): Split out into...
> 	(cp_parser_error_1): ...this new function, merging in content
> 	from...
> 	(cp_parser_required_error): ...here.  Eliminate partial
> duplicate
> 	of body of cp_parser_error in favor of a call to the new
> 	cp_parser_error_1 helper function.
> 
> gcc/testsuite/ChangeLog:
> 	* g++.dg/parse/pragma2.C: Update to reflect reinstatement of
> the
> 	"#pragma is not allowed here" error.
> ---
>  gcc/cp/parser.c                      | 169 ++++++++++++++++++++-----
> ----------
>  gcc/testsuite/g++.dg/parse/pragma2.C |   4 +-
>  2 files changed, 97 insertions(+), 76 deletions(-)
> 
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index 25b91df..56d9442 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -2767,53 +2767,116 @@ cp_lexer_peek_conflict_marker (cp_lexer
> *lexer, enum cpp_ttype tok1_kind,
>    return true;
>  }
>  
> -/* If not parsing tentatively, issue a diagnostic of the form
> +/* Get a description of the matching symbol to TOKEN_DESC e.g. "("
> for
> +   RT_CLOSE_PAREN.  */
> +
> +static const char *
> +get_matching_symbol (required_token token_desc)
> +{
> +  switch (token_desc)
> +    {
> +    default:
> +      gcc_unreachable ();
> +      return "";
> +    case RT_CLOSE_BRACE:
> +      return "{";
> +    case RT_CLOSE_PAREN:
> +      return "(";
> +    }
> +}
> +
> +/* Subroutine of cp_parser_error and cp_parser_required_error.
> +
> +   Issue a diagnostic of the form
>        FILE:LINE: MESSAGE before TOKEN
>     where TOKEN is the next token in the input stream.  MESSAGE
>     (specified by the caller) is usually of the form "expected
> -   OTHER-TOKEN".  */
> +   OTHER-TOKEN".
> +
> +   This bypasses the check for tentative passing, and potentially
> +   adds material needed by cp_parser_required_error.
> +
> +   If MISSING_TOKEN_DESC is not RT_NONE, and MATCHING_LOCATION is
> not
> +   UNKNOWN_LOCATION, then we have an unmatched symbol at
> +   MATCHING_LOCATION; highlight this secondary location.  */
>  
>  static void
> -cp_parser_error (cp_parser* parser, const char* gmsgid)
> +cp_parser_error_1 (cp_parser* parser, const char* gmsgid,
> +		   required_token missing_token_desc,
> +		   location_t matching_location)
>  {
> -  if (!cp_parser_simulate_error (parser))
> +  cp_token *token = cp_lexer_peek_token (parser->lexer);
> +  /* This diagnostic makes more sense if it is tagged to the line
> +     of the token we just peeked at.  */
> +  cp_lexer_set_source_position_from_token (token);
> +
> +  if (token->type == CPP_PRAGMA)
>      {
> -      cp_token *token = cp_lexer_peek_token (parser->lexer);
> -      /* This diagnostic makes more sense if it is tagged to the
> line
> -	 of the token we just peeked at.  */
> -      cp_lexer_set_source_position_from_token (token);
> +      error_at (token->location,
> +		"%<#pragma%> is not allowed here");
> +      cp_parser_skip_to_pragma_eol (parser, token);
> +      return;
> +    }
>  
> -      if (token->type == CPP_PRAGMA)
> +  /* If this is actually a conflict marker, report it as such.  */
> +  if (token->type == CPP_LSHIFT
> +      || token->type == CPP_RSHIFT
> +      || token->type == CPP_EQ_EQ)
> +    {
> +      location_t loc;
> +      if (cp_lexer_peek_conflict_marker (parser->lexer, token->type, 
> &loc))
>  	{
> -	  error_at (token->location,
> -		    "%<#pragma%> is not allowed here");
> -	  cp_parser_skip_to_pragma_eol (parser, token);
> +	  error_at (loc, "version control conflict marker in file");
>  	  return;
>  	}
> +    }
>  
> -      /* If this is actually a conflict marker, report it as
> such.  */
> -      if (token->type == CPP_LSHIFT
> -	  || token->type == CPP_RSHIFT
> -	  || token->type == CPP_EQ_EQ)
> -	{
> -	  location_t loc;
> -	  if (cp_lexer_peek_conflict_marker (parser->lexer, token-
> >type, &loc))
> -	    {
> -	      error_at (loc, "version control conflict marker in
> file");
> -	      return;
> -	    }
> -	}
> +  gcc_rich_location richloc (input_location);
> +
> +  bool added_matching_location = false;
> +
> +  if (missing_token_desc != RT_NONE)
> +    {
> +      /* If matching_location != UNKNOWN_LOCATION, highlight it.
> +	 Attempt to consolidate diagnostics by printing it as a
> +	secondary range within the main diagnostic.  */
> +      if (matching_location != UNKNOWN_LOCATION)
> +	added_matching_location
> +	  = richloc.add_location_if_nearby (matching_location);
> +    }
> +
> +  /* Actually emit the error.  */
> +  c_parse_error (gmsgid,
> +		 /* Because c_parser_error does not understand
> +		    CPP_KEYWORD, keywords are treated like
> +		    identifiers.  */
> +		 (token->type == CPP_KEYWORD ? CPP_NAME : token-
> >type),
> +		 token->u.value, token->flags, &richloc);
>  
> -      rich_location richloc (line_table, input_location);
> -      c_parse_error (gmsgid,
> -		     /* Because c_parser_error does not understand
> -			CPP_KEYWORD, keywords are treated like
> -			identifiers.  */
> -		     (token->type == CPP_KEYWORD ? CPP_NAME : token-
> >type),
> -		     token->u.value, token->flags, &richloc);
> +  if (missing_token_desc != RT_NONE)
> +    {
> +      /* If we weren't able to consolidate matching_location, then
> +	 print it as a secondary diagnostic.  */
> +      if (matching_location != UNKNOWN_LOCATION
> +	  && !added_matching_location)
> +	inform (matching_location, "to match this %qs",
> +		get_matching_symbol (missing_token_desc));
>      }
>  }
>  
> +/* If not parsing tentatively, issue a diagnostic of the form
> +      FILE:LINE: MESSAGE before TOKEN
> +   where TOKEN is the next token in the input stream.  MESSAGE
> +   (specified by the caller) is usually of the form "expected
> +   OTHER-TOKEN".  */
> +
> +static void
> +cp_parser_error (cp_parser* parser, const char* gmsgid)
> +{
> +  if (!cp_parser_simulate_error (parser))
> +    cp_parser_error_1 (parser, gmsgid, RT_NONE, UNKNOWN_LOCATION);
> +}
> +
>  /* Issue an error about name-lookup failing.  NAME is the
>     IDENTIFIER_NODE DECL is the result of
>     the lookup (as returned from cp_parser_lookup_name).  DESIRED is
> @@ -27960,24 +28023,6 @@ cp_parser_friend_p (const
> cp_decl_specifier_seq *decl_specifiers)
>    return decl_spec_seq_has_spec_p (decl_specifiers, ds_friend);
>  }
>  
> -/* Get a description of the matching symbol to TOKEN_DESC e.g. "("
> for
> -   RT_CLOSE_PAREN.  */
> -
> -static const char *
> -get_matching_symbol (required_token token_desc)
> -{
> -  switch (token_desc)
> -    {
> -    default:
> -      gcc_unreachable ();
> -      return "";
> -    case RT_CLOSE_BRACE:
> -      return "{";
> -    case RT_CLOSE_PAREN:
> -      return "(";
> -    }
> -}
> -
>  /* Issue an error message indicating that TOKEN_DESC was expected.
>     If KEYWORD is true, it indicated this function is called by
>     cp_parser_require_keword and the required token can only be
> @@ -28155,31 +28200,7 @@ cp_parser_required_error (cp_parser *parser,
>      }
>  
>    if (gmsgid)
> -    {
> -      /* Emulate rest of cp_parser_error.  */
> -      cp_token *token = cp_lexer_peek_token (parser->lexer);
> -      cp_lexer_set_source_position_from_token (token);
> -
> -      gcc_rich_location richloc (input_location);
> -
> -      /* If matching_location != UNKNOWN_LOCATION, highlight it.
> -	 Attempt to consolidate diagnostics by printing it as a
> -	secondary range within the main diagnostic.  */
> -      bool added_matching_location = false;
> -      if (matching_location != UNKNOWN_LOCATION)
> -	added_matching_location
> -	  = richloc.add_location_if_nearby (matching_location);
> -
> -      c_parse_error (gmsgid,
> -		     (token->type == CPP_KEYWORD ? CPP_NAME : token-
> >type),
> -		     token->u.value, token->flags, &richloc);
> -
> -      /* If we weren't able to consolidate matching_location, then
> -	 print it as a secondary diagnostic.  */
> -      if (matching_location != UNKNOWN_LOCATION &&
> !added_matching_location)
> -	inform (matching_location, "to match this %qs",
> -		get_matching_symbol (token_desc));
> -    }
> +    cp_parser_error_1 (parser, gmsgid, token_desc,
> matching_location);
>  }
>  
>  
> diff --git a/gcc/testsuite/g++.dg/parse/pragma2.C
> b/gcc/testsuite/g++.dg/parse/pragma2.C
> index 3dc5fc1..c5616ff 100644
> --- a/gcc/testsuite/g++.dg/parse/pragma2.C
> +++ b/gcc/testsuite/g++.dg/parse/pragma2.C
> @@ -4,5 +4,5 @@
>  // does not.
>  int f(int x,
>  #pragma interface  // { dg-error "not allowed here" }
> -      // { dg-bogus "expected identifier" "" { xfail *-*-* } .-1 }
> -      int y);
> +      // The parser gets confused and issues an error on the next
> line.
> +      int y); // { dg-bogus "" "" { xfail *-*-* } }