[PATCH 12/18] widl: remove remaining instances of duptype function and replace with specific dup_pointer_type function for the remaining single valid case

[Zebediah Figura]

On 6/5/19 8:34 PM, Richard Pospesel wrote:
> Signed-off-by: Richard Pospesel <richard at torproject.org>
> ---
>   tools/widl/parser.y    |  2 +-
>   tools/widl/typetree.c  | 22 +++++++++++++++-------
>   tools/widl/typetree.h  |  4 ++--
>   tools/widl/widltypes.h |  2 +-
>   4 files changed, 19 insertions(+), 11 deletions(-)

You're changing around two instances of duptype() here; I'd like to see 
them split into individual patches.

> 
> diff --git a/tools/widl/parser.y b/tools/widl/parser.y
> index 54dd3421c2..25bfa959f8 100644
> --- a/tools/widl/parser.y
> +++ b/tools/widl/parser.y
> @@ -1652,7 +1652,7 @@ static var_t *declare_var(attr_list_t *attrs, decl_spec_t *declspec, const decla
>            * store an offset to the typeformat string in the type object, but
>            * two typeformat strings may be written depending on whether the
>            * pointer is a toplevel parameter or not */
> -        *pt = duptype(*pt, 1);
> +        *pt = dup_pointer_type(*pt);

Well...

I firstly think dup_pointer_type() as a separate function is pretty 
pointless; you can just inline it here.

That said, I know you're not making things any worse with this series as 
far as this hack goes, but as long as we're going down the road of 
identifying types uniquely by their pointers, it behoves us to consider 
what a proper solution for this looks like. Consider that we might be 
duplicating a named typedef here; that could cause problems for us 
later. I think if we really want to make this unique (and that's a good 
guarantee to have), the best approach may be to store two different 
offsets in the type_t structure. Or, perhaps better, store an extra 
offset in the pointer_details structure.

>         }
>       }
>       else if (ptr_attr)
> diff --git a/tools/widl/typetree.c b/tools/widl/typetree.c
> index 7a4c8a50c5..d13bb71893 100644
> --- a/tools/widl/typetree.c
> +++ b/tools/widl/typetree.c
> @@ -30,12 +30,16 @@
>   #include "typetree.h"
>   #include "header.h"
>   
> -type_t *duptype(type_t *t, int dupname)
> +/* this function is only used in declare_var in parser.y, see FIXME note */
> +type_t *dup_pointer_type(type_t *t)
>   {
> -  type_t *d = alloc_type();
> +  type_t *d;
>   
> +  assert(is_ptr(t) && t->details.pointer.def_fc != FC_RP);
> +
> +  d = alloc_type();
>     *d = *t;
> -  if (dupname && t->name)
> +  if (t->name)
>       d->name = xstrdup(t->name);
>   
>     return d;
> @@ -189,15 +193,19 @@ type_t *type_new_pointer(unsigned char pointer_default, type_t *ref, attr_list_t
>   
>   type_t *type_new_alias(const decl_spec_t *ds, const char *name)
>   {
> -    type_t *t = ds->type;
> -    type_t *a = duptype(t, 0);
> +    type_t *a = NULL;
> +
> +    assert(ds != NULL);
> +    assert(name != NULL);
> +
> +    a = alloc_type();
> +    /* copy over all the members before setting alias data */
> +    *a = *ds->type;

This is doing the same thing as duptype(), though. I know it's because 
of the way aliases work, but I don't think that keeping the way aliases 
currently work fits well with this refactor. I would propose to define a 
true alias_t, which simply contains a name and a pointer to the 
underlying type. Most places use type_get_real_type() already, so this 
should be pretty natural...

>   
>       a->name = xstrdup(name);
>       a->attrs = NULL;
>       a->orig = *ds;
>       a->is_alias = TRUE;
> -    /* for pointer types */
> -    a->details = t->details;
>       init_loc_info(&a->loc_info);
>   
>       return a;
> diff --git a/tools/widl/typetree.h b/tools/widl/typetree.h
> index 2b0e8ba42d..da5deb4cef 100644
> --- a/tools/widl/typetree.h
> +++ b/tools/widl/typetree.h
> @@ -53,8 +53,8 @@ type_t *type_coclass_define(type_t *coclass, ifref_list_t *ifaces);
>   int type_is_equal(const type_t *type1, const type_t *type2);
>   const char *type_get_name(const type_t *type, enum name_type name_type);
>   
> -/* FIXME: shouldn't need to export this */
> -type_t *duptype(type_t *t, int dupname);
> +/* copy pointer type to deal with need for duplicate typeformat strings */
> +type_t *dup_pointer_type(type_t *t);
>   
>   /* un-alias the type until finding the non-alias type */
>   static inline type_t *type_get_real_type(const type_t *type)
> diff --git a/tools/widl/widltypes.h b/tools/widl/widltypes.h
> index b3665bc4bb..17477a604f 100644
> --- a/tools/widl/widltypes.h
> +++ b/tools/widl/widltypes.h
> @@ -454,7 +454,7 @@ struct _type_t {
>       struct bitfield_details bitfield;
>     } details;
>     const char *c_name;
> -  decl_spec_t orig;                   /* dup'd types */
> +  decl_spec_t orig;                   /* aliased types */
>     unsigned int typestring_offset;
>     unsigned int ptrdesc;           /* used for complex structs */
>     int typelib_idx;
>