tools/widl/typegen.c pointer initialization

Dan Hipschman dsh at linux.ucla.edu
Sun Oct 28 13:56:57 CDT 2007


On Sun, Oct 28, 2007 at 01:51:34PM +0100, Gerald Pfeifer wrote:
> In tools/widl/typegen.c we have the following snippet
> 
>   static void write_user_tfs(FILE *file, type_t *type, unsigned int *tafsoff)
>   {
>     unsigned int start, absoff, flags;
>     unsigned int align = 0, ualign = 0;
>     const char *name;
>     type_t *utype = get_user_type(type, &name);
> 
> which passes name to get_user_type without initializing it.  get_user_type
> in return has the following piece of code:
> 
>   static type_t *get_user_type(const type_t *t, const char **pname)
>   {
>     for (;;)
>     {
>         type_t *ut = get_attrp(t->attrs, ATTR_WIREMARSHAL);
>         if (ut)
>         {
>             if (pname)
>                 *pname = t->name;
>             return ut;
>         }
> 
> It is unclear to this reader (and various versions of GCC I tested),
> whether we don't really use *name (*pname) uninitialized here.  Now
> it is possible that through some of the, hmm, tricky logic in these
> two functions combined with some environmental guarantees it happens
> that we don't, but it seems better to be proactive here.

The logic is as follows:  We wouldn't be in write_user_tfs if the
predicate is_user_type wasn't true for that type, which means *pname
(name) will always be initialized since it gets set precisely when
get_user_type returns non-NULL (which is the is_user_type test).  I
don't think initializing it to NULL is helpful since it would crash just
the same as if it weren't initialized.  name *must* have a valid
(non-NULL) value.  This may silence a warning, but it's making the logic
of the code more confusing by setting the variable to a value that
doesn't make any sense in that context.  get_user_type is the
initialization.

Better than this would be to put "assert(is_user_type(type));" above the
initializations to convince the programmer at least that name will get
initialized correctly in get_user_type.  If that doesn't convince the
compiler then I don't think it's worth it to try to placate it since
we're supposed to be wrting code for humans, not computers.  Some
projects use a macro like "name IF_LINT(= NULL);" to quiet warnings
while informing the reader that the initialization is not necessary.  I
suppose that's slightly better as well.

I think the real problem is that the code is just not clear enough.
I've been meaning to add asserts.  Where asserts are impractical:
comments.



More information about the wine-devel mailing list