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.