Re: [PATCH v2 06/11] OpenMP: lvalue parsing for map clauses (C++)

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Mar 18, 2022 at 09:26:47AM -0700, Julian Brown wrote:
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -4266,6 +4266,9 @@ cp_parser_new (cp_lexer *lexer)
>    parser->omp_declare_simd = NULL;
>    parser->oacc_routine = NULL;
>  
> +  /* Allow array slice in expression.  */

Better /* Disallow OpenMP array sections in expressions.  */

> +  parser->omp_array_section_p = false;
> +
>    /* Not declaring an implicit function template.  */
>    parser->auto_is_implicit_function_template_parm_p = false;
>    parser->fully_implicit_function_template_p = false;

I think we should figure out when we should temporarily disable
  parser->omp_array_section_p = false;
and restore it afterwards to a saved value.  E.g.
cp_parser_lambda_expression seems like a good candidate, the fact that
OpenMP array sections are allowed say in map clause doesn't mean they are
allowed inside of lambdas and it would be especially hard when the lambda
is defining a separate function and the search for OMP_ARRAY_SECTION
probably wouldn't be able to discover those.
Other spots to consider might be statement expressions, perhaps type
definitions etc.

> @@ -8021,6 +8024,7 @@ cp_parser_postfix_open_square_expression (cp_parser *parser,
>    releasing_vec expression_list = NULL;
>    location_t loc = cp_lexer_peek_token (parser->lexer)->location;
>    bool saved_greater_than_is_operator_p;
> +  bool saved_colon_corrects_to_scope_p;
>  
>    /* Consume the `[' token.  */
>    cp_lexer_consume_token (parser->lexer);
> @@ -8028,6 +8032,9 @@ cp_parser_postfix_open_square_expression (cp_parser *parser,
>    saved_greater_than_is_operator_p = parser->greater_than_is_operator_p;
>    parser->greater_than_is_operator_p = true;
>  
> +  saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p;
> +  parser->colon_corrects_to_scope_p = false;

I think the last above line should be guarded on
  if (parser->omp_array_section_p)
There is no reason to get worse diagnostics in non-OpenMP code or even in
OpenMP code where array sections aren't allowed.

> +
> +      /* NOTE: We are reusing using the type of the whole array as the type of
> +	 the array section here, which isn't necessarily entirely correct.
> +	 Might need revisiting.  */

"reusing using" looks weird.
As for the type of OMP_ARRAY_SECTION trees, perhaps we could initially use
an incomplete array (so array element would be meaningful)
and when we figure out the details and the array section is contiguous
change its type to array type covering it.

> +      return build3_loc (input_location, OMP_ARRAY_SECTION,
> +			 TREE_TYPE (postfix_expression),
> +			 postfix_expression, index, length);
> +    }
> +
> +  parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p;
> +
>    /* Look for the closing `]'.  */
>    cp_parser_require (parser, CPP_CLOSE_SQUARE, RT_CLOSE_SQUARE);
>  
> @@ -36536,7 +36570,7 @@ struct omp_dim
>  static tree
>  cp_parser_omp_var_list_no_open (cp_parser *parser, enum omp_clause_code kind,
>  				tree list, bool *colon,
> -				bool allow_deref = false)
> +				bool map_lvalue = false)
>  {
>    auto_vec<omp_dim> dims;
>    bool array_section_p;
> @@ -36547,12 +36581,95 @@ cp_parser_omp_var_list_no_open (cp_parser *parser, enum omp_clause_code kind,
>        parser->colon_corrects_to_scope_p = false;
>        *colon = false;
>      }
> +  begin_scope (sk_omp, NULL);

Why?  Base-language-wise, clauses don't introduce a new scope
for name-lookup.
And if it is really needed, I'd strongly prefer to either do it solely
for the clauses that might need it, or do begin_scope before first
such clause and finish at the end if it has been introduced.

>    while (1)
>      {
>        tree name, decl;
>  
>        if (kind == OMP_CLAUSE_DEPEND || kind == OMP_CLAUSE_AFFINITY)
>  	cp_parser_parse_tentatively (parser);
> +      else if (map_lvalue && kind == OMP_CLAUSE_MAP)
> +	{

This shouldn't be done just for OMP_CLAUSE_MAP, but for all the
other clauses that accept array sections, including
OMP_CLAUSE_DEPEND, OMP_CLAUSE_AFFINITY, OMP_CLAUSE_MAP, OMP_CLAUSE_TO,
OMP_CLAUSE_FROM, OMP_CLAUSE_INCLUSIVE, OMP_CLAUSE_EXCLUSIVE,
OMP_CLAUSE_USE_DEVICE_ADDR, OMP_CLAUSE_HAS_DEVICE_ADDR,
OMP_CLAUSE_*REDUCTION.
And preferrably, they should be kept in the IL until *finish_omp_clauses,
which should handle those instead of TREE_LIST that represented them before.
Additionally, something should diagnose incorrect uses of OMP_ARRAY_SECTION,
which is everywhere in the expressions but as the outermost node(s),
i.e. for clauses that do allow array sections scan OMP_CLAUSE_DECL after
handling handleable array sections and complain about embedded
OMP_ARRAY_SECTION, including OMP_ARRAY_SECTION say in the lower-bound,
length and/or stride expressions of the valid OMP_ARRAY_SECTION.

For C++ that also means handling OMP_ARRAY_SECTION code in pt.c.

	Jakub