Re: [PATCH 1/2] C++: more location wrapper nodes (PR c++/43064, PR c++/43486)

[David Malcolm <dmalcolm at redhat dot com>]

On Wed, 2018-12-19 at 20:00 +0100, Thomas Schwinge wrote:
> Hi David!
> 
> I will admit that I don't have researched ;-/ what this is actually
> all
> about, and how it's implemented, but...
> 
> On Mon,  5 Nov 2018 15:31:08 -0500, David Malcolm <dmalcolm@redhat.co
> m> wrote:
> > The C++ frontend gained various location wrapper nodes in r256448
> > (GCC 8).
> > That patch:
> >   https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00799.html
> > added wrapper nodes around all nodes with !CAN_HAVE_LOCATION_P for:
> > 
> > * arguments at callsites, and for
> > 
> > * typeid, alignof, sizeof, and offsetof.
> > 
> > This is a followup to that patch, adding many more location
> > wrappers
> > to the C++ frontend.  It adds location wrappers for nodes with
> > !CAN_HAVE_LOCATION_P to:
> > 
> > * all literal nodes (in cp_parser_primary_expression)
> > 
> > * all id-expression nodes (in finish_id_expression), except within
> > a
> >   decltype.
> > 
> > * all mem-initializer nodes within a mem-initializer-list
> >   (in cp_parser_mem_initializer)
> > 
> > However, the patch also adds some suppressions: regions in the
> > parser
> > for which wrapper nodes will not be created:
> > 
> > * within a template-parameter-list or template-argument-list (in
> >   cp_parser_template_parameter_list and
> > cp_parser_template_argument_list
> >   respectively), to avoid encoding the spelling location of the
> > nodes
> >   in types.  For example, "array<10>" and "array<10>" are the same
> > type,
> >   despite the fact that the two different "10" tokens are spelled
> > in
> >   different locations in the source.
> > 
> > * within a gnu-style attribute (none of are handlers are set up to
> > cope
> >   with location wrappers yet)
> > 
> > * within various OpenMP clauses

I suppressed the addition of wrapper nodes within OpenMP as a way to
reduce the scope of the patch.

> ... I did wonder why things applicable to OpenMP wouldn't likewise
> apply
> to OpenACC, too?  That is:

It might or might not be.  Maybe there's a gap in my test coverage? 
How should I be running the OpenACC tests?

> > 	(cp_parser_omp_all_clauses): Don't create wrapper nodes within
> > 	OpenMP clauses.
> > 	(cp_parser_omp_for_loop): Likewise.
> > 	(cp_parser_omp_declare_reduction_exprs): Likewise.
> > @@ -33939,6 +33968,9 @@ cp_parser_omp_all_clauses (cp_parser
> > *parser, omp_clause_mask mask,
> >    bool first = true;
> >    cp_token *token = NULL;
> >  
> > +  /* Don't create location wrapper nodes within OpenMP
> > clauses.  */
> > +  auto_suppress_location_wrappers sentinel;
> > +
> >    while (cp_lexer_next_token_is_not (parser->lexer,
> > CPP_PRAGMA_EOL))
> >      {
> >        pragma_omp_clause c_kind;
> > @@ -35223,6 +35255,10 @@ cp_parser_omp_for_loop (cp_parser *parser,
> > enum tree_code code, tree clauses,
> >  	}
> >        loc = cp_lexer_consume_token (parser->lexer)->location;
> >  
> > +      /* Don't create location wrapper nodes within an OpenMP
> > "for"
> > +	 statement.  */
> > +      auto_suppress_location_wrappers sentinel;
> > +
> >        matching_parens parens;
> >        if (!parens.require_open (parser))
> >  	return NULL;
> > @@ -37592,6 +37628,8 @@ cp_parser_omp_declare_reduction_exprs (tree
> > fndecl, cp_parser *parser)
> >        else
> >  	{
> >  	  cp_parser_parse_tentatively (parser);
> > +	  /* Don't create location wrapper nodes here.  */
> > +	  auto_suppress_location_wrappers sentinel;
> >  	  tree fn_name = cp_parser_id_expression (parser,
> > /*template_p=*/false,
> >  						  /*check_dependen
> > cy_p=*/true,
> >  						  /*template_p=*/N
> > ULL,
> 
> Shouldn't "cp_parser_oacc_all_clauses" (and "some" other functions?)
> be
> adjusted in the same way?  How would I test that?  (I don't see any
> OpenMP test cases added -- I have not yet tried whether any problems
> would become apparent when temporarily removing the OpenMP changes
> cited
> above.)

Lots of pre-existing OpenMP test cases started failing when I added the
wrapper nodes to the C++ parser (e.g. for id-expressions and
constants); suppressing them in the given places was an easy way to get
them to pass again.

Dave