Re: widl [3/6]: Represent typedefs with easier to use structures

[Robert Shearman]

Dan Hipschman wrote:
> diff --git a/tools/widl/header.c b/tools/widl/header.c
> index 3a4cea9..5de43c0 100644
> --- a/tools/widl/header.c
> +++ b/tools/widl/header.c
> @@ -203,13 +203,14 @@ void write_type(FILE *h, type_t *t, cons
> {
>   int c;
>
> +  if (t->is_const) fprintf(h, "const ");
> +
>   if (n) fprintf(h, "%s", n);
> +  else if (t->kind == TKIND_ALIAS) fprintf(h, "%s", t->name);

I'm not really sure we should be depending on a typelib thing for a 
fundamental concept such as this. In fact, I think the kind field of the 
type_t structure could probably be removed and generated as needed in 
the typelib generation code.
> @@ -902,6 +885,7 @@ static type_t *make_int(int sign)
>
> void init_types(void)
> {
> +  decl_builtin("void", 0);
>   decl_builtin("byte", RPC_FC_BYTE);
>   decl_builtin("wchar_t", RPC_FC_WCHAR);
>   decl_builtin("int", RPC_FC_LONG);     /* win32 */

This would seem as though it would make widl incorrectly accept "int 
func(void a);" as a valid statement. I think you need to try to fix this 
case.
> @@ -1147,6 +1131,7 @@ static type_t *make_type(unsigned char t
>   t->type = type;
>   t->ref = ref;
>   t->attrs = NULL;
> +  t->orig = NULL;
>   t->funcs = NULL;
>   t->fields = NULL;
>   t->ifaces = NULL;
> @@ -1175,8 +1160,15 @@ static typeref_t *uniq_tref(typeref_t *r
>   typeref_t *t = ref;
>   type_t *tp;
>   if (t->uniq) return t;
> -  tp = make_type(0, t->ref);
> -  tp->name = t->name;
> +
> +  if (t->name)
> +  {
> +    tp = duptype(t->ref, 0);
> +    tp->name = t->name;
> +  }
> +  else
> +    tp = duptype(t->ref, 1);
> +
>   t->name = NULL;
>   t->ref = tp;
>   t->uniq = 1;
> @@ -1311,11 +1303,27 @@ static unsigned char get_pointer_type( t
>   return RPC_FC_FP;
> }
>
> -static type_t *reg_types(type_t *type, var_t *names, int t)
> +static type_t *reg_typedefs(type_t *type, var_t *names, attr_t *attrs)
> {
>   type_t *ptr = type;
>   int ptrc = 0;
>
> +  /* We must generate names for tagless enum, struct or union.
> +     Typedef-ing a tagless enum, struct or union means we want the typedef
> +     to be included in a library whether it has other attributes or not,
> +     hence the public attribute.  */
> +  if ((type->kind == TKIND_ENUM || type->kind == TKIND_RECORD
> +       || type->kind == TKIND_UNION) && ! type->name && ! parse_only)
> +  {
> +    if (! is_attr(attrs, ATTR_PUBLIC))
> +    {
> +      attr_t *new_attrs = make_attr(ATTR_PUBLIC);
> +      LINK(new_attrs, attrs);
> +      attrs = new_attrs;
> +    }

I'm not sure it is necessary to include this change with the rest of the 
patch. I'm also not sure what you are trying to accomplish here (as we 
always add the type to a typelib, no matter whether the type has the 
public attribute).
> +    type->name = gen_name();
> +  }
> +
>   while (names) {
>     var_t *next = NEXT_LINK(names);
>     if (names->name) {
> @@ -1323,8 +1331,7 @@ static type_t *reg_types(type_t *type, v
>       int cptr = names->ptr_level;
>       if (cptr > ptrc) {
>         while (cptr > ptrc) {
> -          int t = get_pointer_type( cur );
> -          cur = ptr = make_type(t, cur);
> +          cur = ptr = make_type(RPC_FC_RP, cur);
>           ptrc++;
>         }
>       } else {

Thanks and keep up the good work!

-- 
Rob Shearman

widl [3/6]: Represent typedefs with easier to use structures

[Dan Hipschman]

On Tue, Aug 29, 2006 at 01:45:41PM +0100, Robert Shearman wrote:
> > if (n) fprintf(h, "%s", n);
> > +  else if (t->kind == TKIND_ALIAS) fprintf(h, "%s", t->name);
>
> I'm not really sure we should be depending on a typelib thing for a 
> fundamental concept such as this. In fact, I think the kind field of the 
> type_t structure could probably be removed and generated as needed in 
> the typelib generation code.

The kind field is useful outside of the typelib, though.  In fact, as you
say, it's not really even that useful inside the typelib code.  It's *more*
useful in the parser and header code.  If you don't like me using it this
way, maybe we could just rename the field and constants to remove the
connotation of being associated only with typelibs.  Without using the
"type == 0 && ref != NULL" combo to designate typedefs, we need some way
to separate typedefs from regular types, and this was the most obvious
choice for me.
> > void init_types(void)
> > {
> > +  decl_builtin("void", 0);
> >  decl_builtin("byte", RPC_FC_BYTE);
>
> This would seem as though it would make widl incorrectly accept "int 
> func(void a);" as a valid statement. I think you need to try to fix this 
> case.

This patch doesn't change the grammar or semantic phase at all.  It only
removes a special case in write_type so we can print void types by their
name field.  The point was to allow this chunk of patch:

-      }
-    }
-    else {
-      if (t->ref) {
-        write_type(h, t->ref, NULL, t->name);
-      }
-      else fprintf(h, "void");

Actually, the "int f(void a)" declaration is already accepted, even without
this patch.  This patch doesn't fix that; it wasn't intended to change the
language semantics at all.  Now that you bring it up, however, I, or
somebody else, can submit a patch that will fix the problem fairly easily,
I think.
> > -static type_t *reg_types(type_t *type, var_t *names, int t)
> > +static type_t *reg_typedefs(type_t *type, var_t *names, attr_t *attrs)
> > {
> >  type_t *ptr = type;
> >  int ptrc = 0;
> >
> > +  /* We must generate names for tagless enum, struct or union.
> > +     Typedef-ing a tagless enum, struct or union means we want the typedef
> > +     to be included in a library whether it has other attributes or not,
> > +     hence the public attribute.  */
> > +  if ((type->kind == TKIND_ENUM || type->kind == TKIND_RECORD
> > +       || type->kind == TKIND_UNION) && ! type->name && ! parse_only)
> > +  {
> > +    if (! is_attr(attrs, ATTR_PUBLIC))
> > +    {
> > +      attr_t *new_attrs = make_attr(ATTR_PUBLIC);
> > +      LINK(new_attrs, attrs);
> > +      attrs = new_attrs;
> > +    }
>
> I'm not sure it is necessary to include this change with the rest of the 
> patch. I'm also not sure what you are trying to accomplish here (as we 
> always add the type to a typelib, no matter whether the type has the 
> public attribute).

I need to generate names for tagless structs, etc., because my patch changes
the way typedefs are output in the header file.  Currently,

typedef struct { int x; } foo, bar;

will be output exactly as is in the header (disregarding whitespace changes).
However, this patch splits the typedefs up like this:

typedef struct { int x; } foo;
typedef struct ??? bar;

This is why we need the generated names.  As for adding the public attribute,
we don't (and MIDL doesn't) add typedefs to the typelib if they don't have any
attributes (since reg_types was only used for typedefs, I renamed it to
reg_typedefs to make the use clearer).  By observing MIDL, I found that it will
add the public attributes to typdefs on tagless structs, etc., so they are
included in the typelib.

Thanks for your comments.  I'm trying my best to improve widl, so if you see
something that looks questionable, I don't mind the criticism.

widl [3/6]: Represent typedefs with easier to use structures

[Dan Kegel]

On 8/29/06, Dan Hipschman <dsh(a)linux.ucla.edu> wrote:
> I need to generate names for tagless structs, etc., because my patch changes
> the way typedefs are output in the header file.  Currently,
>
> typedef struct { int x; } foo, bar;
>
> will be output exactly as is in the header (disregarding whitespace changes).
> However, this patch splits the typedefs up like this:
>
> typedef struct { int x; } foo;
> typedef struct ??? bar;
>
> This is why we need the generated names.

I think we might need them anyway; standard C++ is very picky, and requires
tags in more circumstances than one would like.