Re: [PATCH v2] c++: parser - Support for target address spaces in C++

[Paul Iannetta <piannetta@kalrayinc.com>]

On Thu, Oct 13, 2022 at 07:46:46AM +0200, Jakub Jelinek wrote:
> On Thu, Oct 13, 2022 at 02:52:59AM +0200, Paul Iannetta via Gcc-patches wrote:
> > +		if (type != error_mark_node
> > +		    && !ADDR_SPACE_GENERIC_P (TYPE_ADDR_SPACE (type))
> > +		    && current_function_decl)
> > +		  {
> > +		    error
> > +		      ("compound literal qualified by address-space qualifier");
> > +		    type = error_mark_node;
> 
> Can you please write this as:
> 		    error ("compound literal qualified by address-space "
> 			   "qualifier");
> ?  That is how diagnostics that don't fit on one line are usually written.
> 
> > @@ -23812,6 +23830,11 @@ cp_parser_cv_qualifier_seq_opt (cp_parser* parser)
> >  	  break;
> >  	}
> >  
> > +      if (RID_FIRST_ADDR_SPACE <= token->keyword &&
> 
> && should never go at the end of line.
> 
> > +	  token->keyword <= RID_LAST_ADDR_SPACE)
> > +	cv_qualifier =
> 
> and similarly = (except for aggregate initializers).
> 
> > +	  ENCODE_QUAL_ADDR_SPACE (token->keyword - RID_FIRST_ADDR_SPACE);
> 
> So:
> 
>       if (RID_FIRST_ADDR_SPACE <= token->keyword
> 	  && token->keyword <= RID_LAST_ADDR_SPACE)
> 	cv_qualifier
> 	  = ENCODE_QUAL_ADDR_SPACE (token->keyword - RID_FIRST_ADDR_SPACE);
> 
> > +	  int unified_cv =
> > +	    CLEAR_QUAL_ADDR_SPACE (arg_cv_quals & ~parm_cv_quals)
> > +	    | ENCODE_QUAL_ADDR_SPACE (as_common);
> 
> Similarly (but this time with ()s added to ensure correct formatting in
> some editors).
> 
> 	  int unified_cv
> 	    = (CLEAR_QUAL_ADDR_SPACE (arg_cv_quals & ~parm_cv_quals)
> 	       | ENCODE_QUAL_ADDR_SPACE (as_common));
> 
> >        result_type
> >  	= cp_build_qualified_type (void_type_node,
> > -				   (cp_type_quals (TREE_TYPE (t1))
> > -				    | cp_type_quals (TREE_TYPE (t2))));
> > +				   (CLEAR_QUAL_ADDR_SPACE (cp_type_quals (TREE_TYPE (t1)))
> > +				    | CLEAR_QUAL_ADDR_SPACE (cp_type_quals (TREE_TYPE (t2)))
> 
> The above 2 lines are way too long.
> I'd suggest to use temporaries, say
> 	  int quals1 = cp_type_quals (TREE_TYPE (t1));
> 	  int quals2 = cp_type_quals (TREE_TYPE (t2));
> and use those.
> 
> 	Jakub

Thank you for the style review, I'll apply them in the next iteration.

Paul