Added Kerne32Util method to facilitate checking that calls to LocalFree/GlobalFree are successful

[lgoldstein]

The current code calls Local/GlobalFree without checking that they succeeded.

[dblock]

The syntax of these is - [@name](link)., mind fixing this one and the one above too please?

[lgoldstein]

Done.

[dblock]

I think that in the Util class we only want the version that raises an exception, just for consistency with everything else. Cause if you want to check the error yourself you can call the Kernel32 original API, right? I would also just call these localFree and such?

[lgoldstein]

Normally, I would agree with you - but as you can see from my code changes, most people don't bother to check the return value or the error. By using this piece of code ourselves we give a good example as to the kind of coding standards we recommend. Furthermore, by providing these helper methods we make acceptance and usage of said standards more likely.

Also, specifically for the version with the return code - 2 reasons to keep it (IMO):

1. Unit tests (as shown) - provide more information if free failed (the error code in this case)
2. Perhaps someone wants to handle the error themselves somehow and not necessarily throw an exception - let's not second guess the needs of the users.

[lgoldstein]

@dblock Please let me know if you approve of the request in view of the clarifications since I am waiting on it in order to add a critical fix for issue #604.

[dblock]

Personally I still don't love having both freeLocalMemory and validateFreeLocalMemory, turning an error into an exception is great, but turning an error into another kind of error (via getLastError) is not. The Util classes are all throwing exceptions and never return error codes, this breaks that principle and opens the door for all kinds of wild behavior in the library.

[twall]

I agree on this one, if someone wants an error code they can call GetLastError on their own. Just make freeLocalMemory() raise an exception and get rid of the “validateXXX” variants.

On Mar 6, 2016, at 8:46 AM, Daniel Doubrovkine (dB.) @dblockdotorg [email protected] wrote:

Personally I still don't love having both freeLocalMemory and validateFreeLocalMemory, turning an error into an exception is great, but turning an error into another kind of error (via getLastError) is not. The Util classes are all throwing exceptions and never return error codes, this breaks that principle and opens the door for all kinds of wild behavior in the library.

—
Reply to this email directly or view it on GitHub.

[lgoldstein]

but turning an error into another kind of error (via getLastError) is not

The code does not turn one error into another - it simply gives the programmer the choice to examine the returned error code and decide on another kind of handling rather than throwing an exception.

Just make freeLocalMemory() raise an exception and get rid of the “validateXXX” variants

If you feel this strongly about this I will do it (as a separate PR).

[dblock]

@lgoldstein Yes, you're right, it gives the programmer the choice to examine the returned error code. I fairly strongly feel that the programmer should not have that choice in a Util class as a matter of consistency.

[lgoldstein]

Very well then, I will publish a PR that adheres to this convention soon (unless you'd rather I do it directly on the master branch)