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.