Dmitry Timoshkov wrote:
Certainly RegCloseKey and HeapFree can fail if someone will feed garbage to them, but if you really know that parameters you pass are valid, there is no reason to check the returned values. Please use simple RegCloseKey and HeapFree instead of monster macros.
This is certainly something I didn't expect. I don't want to get into a religious war over coding style, but I'm not comfortable ignoring the return value of a function that can fail -- no matter how unlikely that failure is.
- I do not really know that the parameters I am passing are valid. OK, I'm pretty darn sure in this case, but I guarantee that the day I make that assumption will be the day I pass the wrong variable or make some similarly stupid mistake.
- My understanding is that the goal of the Wine project is to reverse engineer the Win32 API. If this is true, the Win32 API documen- tation is the "contract" between the author of a function (such as HeapFree) and the caller of that function. Since the documentation does not mention the circumstances under which the call will fail, assuming that it doesn't fail is relying on an undocumented behavior. (Even if I examine the source code of the Wine functions that I call and ensure that the current implementations cannot fail when I call them, there is no guarantee that this behavior will not change in the future.)
- Consider the actual impact of this check. In each case it's a comparison of a 32-bit return value (presumably a register on all platforms) to 0. Assuming a successful call to the ASCII version of this function, this is a total of six comparisons to 0. If Wine is compiled with warnings disabled, the compiler should optimize out the comparisons entirely.
As to "monster" macros, I could have written each in one or two lines, but I have found that C constructs of the "if (!(r = function x)))" form cause me an inordinate amount of trouble when I have to analyze them later. (Note that I am *not* telling other people not to use such constructs. I'll respect your coding style, but please grant me the same courtesy.) BTW, I chose macros rather than inline functions so that any warnings generated would show the correct function name.
I hope this clarifies my reasoning somewhat. Of course it's not ultimately my decision what gets committed, but I think that the arguments for checking the return value are compelling, and I think that the use of macros greatly increases the readability of the code.
Thanks!