Re: [libvirt] [PATCH 6/7] xend_internal.c: assure clang that we do not dereference NULL

On 04/14/2010 11:29 AM, Jim Meyering wrote: >>> } else if (STREQ(type, "tcp")) { >>> + sa_assert (value); >>> const char *offset = strchr(value, ':'); >> >> This introduces an expression before a declaration, even when sa_assert >> is empty. I know that we already rely on several other C99 features >> (like __VA_ARGS__ from the preprocessor), but my understanding has been >> that we have been trying to stick with C89 declarations first still. >> Does this need to be refactored accordingly? > > There have been others like this, and no one objected then. > Prohibiting stmt-after-decl is detrimental. In re-reading my comment, I can see my position was not clear. I am very much in favor of relying on more C99 features, especially decl-after-stmt. I'm not opposed to your solution, so much as questioning whether others are still trying to stick to the C89 rules, as well as point out that sticking to C89 in this case would be anachronistic given that we already have rely on other C99 features like // for comments. >> Besides, xend_parse_sexp_desc_char already dereferences value at line >> 1224; > > True, but just after that, it may be set to NULL: > > if (value[0] == '/') { > type = "dev"; > } else if (STRPREFIX(value, "null")) { > type = "null"; > value = NULL; <<<<=================== Ahh. My review was not thorough enough, then. Still, clang didn't complain about the first value[0] dereference?

At any rate, your additional explanation, coupled with my second review of that function, is enough to warrant ACK, once 1/7 is resolved.