Re: [PATCH v3 02/11] [PR gdb/14441] gdb: gdbtypes: change {lookup,make}_reference_type() API

[Artemiy Volkov <artemiyv at acm dot org>]

On Wed, Mar 16, 2016 at 03:19:13PM -0700, Keith Seitz wrote:

[snip]

> > +   the kind of reference type to lookup (lvalue or rvalue reference).  */
> >  
> >  struct type *
> > -make_reference_type (struct type *type, struct type **typeptr)
> > +make_reference_type (struct type *type, struct type **typeptr,
> > +                      enum type_code refcode)
> >  {
> >    struct type *ntype;	/* New type */
> > +  struct type **reftype;
> >    struct type *chain;
> >  
> > -  ntype = TYPE_REFERENCE_TYPE (type);
> > +  gdb_assert (refcode == TYPE_CODE_REF || refcode == TYPE_CODE_RVALUE_REF);
> > +
> > +  ntype = (refcode == TYPE_CODE_REF ? TYPE_REFERENCE_TYPE (type)
> > +           : TYPE_RVALUE_REFERENCE_TYPE (type));
> >  
> >    if (ntype)
> >      {
> > @@ -421,7 +427,11 @@ make_reference_type (struct type *type, struct type **typeptr)
> >      }
> >  
> >    TYPE_TARGET_TYPE (ntype) = type;
> > -  TYPE_REFERENCE_TYPE (type) = ntype;
> > +  reftype = (refcode == TYPE_CODE_REF ? &TYPE_REFERENCE_TYPE (type)
> > +             : &TYPE_RVALUE_REFERENCE_TYPE (type));
> > +
> > +  if(*reftype != NULL)
> > +    *reftype = ntype;
> >  
> >    /* FIXME!  Assume the machine has only one representation for
> >       references, and that it matches the (only) representation for
> 
> I would prefer to do:
> 
>   if (refcode == TYPE_CODE_REF)
>     TYPE_REFERENCE_TYPE (type) = ntype;
>   else
>     TYPE_RVALUE_REFERENCE_TYPE (type) = ntype;
> 
> It's cleaner/smaller/easier to read (at least for me). [Otherwise, a
> space is missing between "if" and "(".] See next comment, though.

My idea was to create a new variable 'reftype' and use it as a
pointer to the member of struct main_type that we're dealing with here.
Without it, we would have to paste your fragment (more readable,
granted) two times rather than just do "*reftype = ntype" two times. I
think the latter is better since it's several lines shorter in total and
reduces the amount of copy-paste code.

> 
> 
> > @@ -429,10 +439,9 @@ make_reference_type (struct type *type, struct type **typeptr)
> >  
> >    TYPE_LENGTH (ntype) =
> >      gdbarch_ptr_bit (get_type_arch (type)) / TARGET_CHAR_BIT;
> > -  TYPE_CODE (ntype) = TYPE_CODE_REF;
> > +  TYPE_CODE (ntype) = refcode;
> >  
> > -  if (!TYPE_REFERENCE_TYPE (type))	/* Remember it, if don't have one.  */
> > -    TYPE_REFERENCE_TYPE (type) = ntype;
> > +  *reftype = ntype;
> >  
> 
> This is slightly different from the original, which only set the new
> type if it was unset in the type. Your revised version will
> unconditionally do it every time. I have no idea if it makes a
> difference or not. Do you?

AFAICT *reftype is provably unequal to 0 here, because it was set to
ntype by the previous assignment, and ntype was unequal to 0 itself. I
went ahead and simplified this a little bit. Is this kind of code
change justified here?

> 
> >    /* Update the length of all the other variants of this type.  */
> >    chain = TYPE_CHAIN (ntype);

Thanks!
Artemiy