While the great alloca debate rages on, here is a version of the patch that uses HeapAlloc for all dynamic allocation.
(FWIW, my personal take is that if alloca causes the program to crash when it fails, then it's not really useful. I only used it, because the GNU libc documentation indicates that it returns NULL when it fails.)
If no one has any other feedback on the patch, I'll submit it to wine- patches tomorrow. (So speak now or forever hold your peace.)
"Ian Pilcher" pilcher@concentric.net wrote:
+/*
- The Win32 doc says that RegCloseKey and HeapFree can fail, but it's hard to
- imagine why either of them would. And what is the calling function supposed
- to do about it anyway?
- */
+#define REGCLOSEKEY(X) \ +{ \
- DWORD ret; \
- \
- ret = RegCloseKey (X); \
- if (ret != ERROR_SUCCESS) \
- WARN ("RegCloseKey returned %li\n", ret); \
+}
+#define HEAPFREE(X,Y,Z) \ +{ \
- if (HeapFree ((X), (Y), (Z)) == 0) \
- WARN ("HeapFree failed with code %li\n", GetLastError ()); \
+}
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.
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!
"Ian Pilcher" pilcher@concentric.net wrote:
- 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.
You *really know* that the parameters you pass are valid, because you did check the value of the call that did return that value. Isn't it?
- My understanding is that the goal of the Wine project is to reverse engineer the Win32 API.
Not really. Reverse engineering is not legal in some countries. The Wine project does *implementation* of 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.
Available Win32 API documentation is not more than "the feed for the thought". All behind that is achieved via writing test apps and some other things: learning traces, debugging and (sometimes) disassembling (sort of).
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.
Since *any* API call theoretically could fail, are you going to check all the returned values? Some reasonable approach could save a lot of time and lines of code.
(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.
Even if the call to HeapFree or RegCloseKey was failed, what can we do? Nothing.
Dmitry Timoshkov wrote:
You *really know* that the parameters you pass are valid, because you did check the value of the call that did return that value. Isn't it?
In theory, yes. But what about the case of a mistake on my part (or the part of the person who implemented the underlying call)? We all do everything possible to produce error-free code, but mistakes *do* happen.
There could also be legitimate reasons for these calls to fail, even with valid inputs. I don't know that this is the case, but it might be entirely correct for HeapFree to fail if another thread has corrupted the process heap.
Not really. Reverse engineering is not legal in some countries. The Wine project does *implementation* of the Win32 API.
I don't think that this affects the validity of my arguments.
Available Win32 API documentation is not more than "the feed for the thought". All behind that is achieved via writing test apps and some other things: learning traces, debugging and (sometimes) disassembling (sort of).
I agree that this is true for the implementor of a specific API. In fact, I had to entirely rethink the ASCII implementation of EnumPrinterDataEx when I realized that Windows 2000 returned ASCII and Unicode strings in the same size buffers. Basically, I did extra work to match the undocumented behavior.
Now consider what happens when I turn around and use EnumPrinterDataEx in the PostScript driver. It would be completely inappropriate for me to write code that relies on this undocumented behavior, even though I implemented it.
The point is that there has to be a "contract". Without it, we don't have an API; we have a bunch of functions. If the Win32 documentation isn't our "contract", what is?
Since *any* API call theoretically could fail, are you going to check all the returned values? Some reasonable approach could save a lot of time and lines of code.
Well . . . yes.
What kind of time are we saving? I think that the processor time consumed (only when Wine is compiled with warnings on) is entirely worth it if it helps catch a single error. Developer's time? I think it's far easier to check the value returned than to adequately research the exact circumstances under which each call might fail.
As far as lines of code, that's why I used the macros.
Even if the call to HeapFree or RegCloseKey was failed, what can we do? Nothing.
We can't gracefully recover, but I think it's entirely appropriate to issue a warning -- if that's what the user has requested through their compile/runtime options.
As this discussion goes on, please keep in mind that I'm not asking anyone else to adopt my views or style, no matter how good I think the reasons for them are.