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.
On Sun, 28 Oct 2007, Dan Hipschman wrote:
The logic is as follows:
Thanks for the explanation, Dan!
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.
I think the question is whether a compiler can reasonably be expected to deduce that the source is fine. If that deduction involves solving the halting problem (or similar) hacking the source to avoid the warning actually doesn't occur to be that bad. ;-)
After the work I did again in the last weeks Wine now builds more or less without warnings for most versions of GCC (and our default set of options). If things continue to proceed well, this may be the only warning left in the whole codebase in a week or two.
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.
Does this mean you are going to submit some patches to address this?
Gerald
On Mon, Oct 29, 2007 at 01:26:45AM +0100, Gerald Pfeifer wrote:
I think the question is whether a compiler can reasonably be expected to deduce that the source is fine. If that deduction involves solving the halting problem (or similar) hacking the source to avoid the warning actually doesn't occur to be that bad. ;-)
Nope, you can't depend on the compiler to verify your code is correct. That's why you should try to write it in such a way that people can understand it. Hacking the code to make the compiler happy at the cost of making the code less clear is not a good idea.
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.
Does this mean you are going to submit some patches to address this?
If you tell me what options you build with and I can reproduce the warning then I'll be more than happy to try to fix it. I build widl with -W -Wall and get two warnings. One of which is in the bison-generated code; the other I sent a patch to silence and Alexandre rejected it.
-
Dan Hipschman -
Gerald Pfeifer