| Message ID | 1450465540-13257-1-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/18/2015 08:05 PM, David Malcolm wrote: > On Thu, 2015-12-17 at 19:21 +0100, Bernd Schmidt wrote: >> On 12/17/2015 07:32 PM, David Malcolm wrote: >>> + if (close_paren_loc) >> >> close_paren_loc != UNKNOWN_LOCATION - it's very confusing otherwise. >> >> >> Bernd > > Here's an updated version; the only change since v1 is: > - if (close_paren_loc) > + if (close_paren_loc != UNKNOWN_LOCATION) > > Have verified the fix in valgrind. OK for trunk if it still passes > bootstrap®rtest? > > gcc/cp/ChangeLog: > * parser.c (cp_parser_postfix_expression): Initialize > close_paren_loc to UNKNOWN_LOCATION; only use it if > it has been written to by > cp_parser_parenthesized_expression_list. > (cp_parser_postfix_dot_deref_expression): Likewise. > (cp_parser_parenthesized_expression_list): Document the behavior > with respect to the CLOSE_PAREN_LOC param. I usually like to leave C++ patches to Jason, but I guess I don't need to know the standard inside out for this one. Prior to your ealier patch, we had protected_set_expr_location (postfix_expression, token->location); It looks like after your new patch, we could end up not setting the location on the expr at all if close_paren_loc is still UNKNOWN_LOCATION. I'm guessing this is intentional as the comment update suggests this is an error path, and the cp_expr constructor ensures that we get at least UNKNOWN_LOCATION and not another uninitialized loc. If I'm correct in all that, the patch is ok. Bernd
On Mon, 2016-01-11 at 16:51 +0100, Bernd Schmidt wrote: > On 12/18/2015 08:05 PM, David Malcolm wrote: > > On Thu, 2015-12-17 at 19:21 +0100, Bernd Schmidt wrote: > >> On 12/17/2015 07:32 PM, David Malcolm wrote: > >>> + if (close_paren_loc) > >> > >> close_paren_loc != UNKNOWN_LOCATION - it's very confusing otherwise. > >> > >> > >> Bernd > > > > Here's an updated version; the only change since v1 is: > > - if (close_paren_loc) > > + if (close_paren_loc != UNKNOWN_LOCATION) > > > > Have verified the fix in valgrind. OK for trunk if it still passes > > bootstrap®rtest? > > > > gcc/cp/ChangeLog: > > * parser.c (cp_parser_postfix_expression): Initialize > > close_paren_loc to UNKNOWN_LOCATION; only use it if > > it has been written to by > > cp_parser_parenthesized_expression_list. > > (cp_parser_postfix_dot_deref_expression): Likewise. > > (cp_parser_parenthesized_expression_list): Document the behavior > > with respect to the CLOSE_PAREN_LOC param. > > I usually like to leave C++ patches to Jason, but I guess I don't need > to know the standard inside out for this one. > > Prior to your ealier patch, we had > > protected_set_expr_location (postfix_expression, token->location); > > It looks like after your new patch, we could end up not setting the > location on the expr at all if close_paren_loc is still > UNKNOWN_LOCATION. I'm guessing this is intentional as the comment update > suggests this is an error path, and the cp_expr constructor ensures that > we get at least UNKNOWN_LOCATION and not another uninitialized loc. Yes. > If I'm correct in all that, the patch is ok. Thanks. Committed to trunk as r232238, having verified bootstrap®rtest, and re-verified the PR under valgrind. Dave
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index a420cf1..ae61d8a 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -6664,7 +6664,7 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p, bool saved_non_integral_constant_expression_p = false; tsubst_flags_t complain = complain_flags (decltype_p); vec<tree, va_gc> *args; - location_t close_paren_loc; + location_t close_paren_loc = UNKNOWN_LOCATION; is_member_access = false; @@ -6826,10 +6826,13 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p, koenig_p, complain); - location_t combined_loc = make_location (token->location, - start_loc, - close_paren_loc); - postfix_expression.set_location (combined_loc); + if (close_paren_loc != UNKNOWN_LOCATION) + { + location_t combined_loc = make_location (token->location, + start_loc, + close_paren_loc); + postfix_expression.set_location (combined_loc); + } /* The POSTFIX_EXPRESSION is certainly no longer an id. */ idk = CP_ID_KIND_NONE; @@ -7298,7 +7301,10 @@ cp_parser_postfix_dot_deref_expression (cp_parser *parser, plain identifier argument, normal_attr for an attribute that wants an expression, or non_attr if we aren't parsing an attribute list. If NON_CONSTANT_P is non-NULL, *NON_CONSTANT_P indicates whether or - not all of the expressions in the list were constant. */ + not all of the expressions in the list were constant. + If CLOSE_PAREN_LOC is non-NULL, and no errors occur, then *CLOSE_PAREN_LOC + will be written to with the location of the closing parenthesis. If + an error occurs, it may or may not be written to. */ static vec<tree, va_gc> * cp_parser_parenthesized_expression_list (cp_parser* parser,