On Wed, Oct 29, 2008 at 7:39 PM, Pete Myers peterdanielmyers@googlemail.com wrote:
In line with http://wiki.winehq.org/ReplaceMalloc this is a small patch that changes all malloc calls to HeapAlloc in the following files: ./dlls/iphlpapi/tests/iphlpapi.c ./dlls/wnaspi32/winaspi16.c
Changelog:
- malloc calls in dlls/kernel32/process.c have been change to HeapAlloc
Your changelog (right above) doesn't match the patch, and
- PMIB_IPADDRTABLE buf = (PMIB_IPADDRTABLE)malloc(dwSize); + PMIB_IPADDRTABLE buf = (PMIB_IPADDRTABLE)HeapAlloc( GetProcessHeap(), 0, dwSize);
You've introduced a space after opening parentheses which does not match the style of the rest of the file.
It does match the style of the way that HeapAlloc is used elsewhere in the file, though not malloc admittedly. I wasn't sure which convention had precedent.
I'm afraid that I don't understand how my changelog doesn't match the patch. I'm new here, can you help me out? There's obviously something I'm missing!
Pete
2008/10/30 James Hawkins truiken@gmail.com
On Wed, Oct 29, 2008 at 7:39 PM, Pete Myers peterdanielmyers@googlemail.com wrote:
In line with http://wiki.winehq.org/ReplaceMalloc this is a small patch
that
changes all malloc calls to HeapAlloc in the following files: ./dlls/iphlpapi/tests/iphlpapi.c ./dlls/wnaspi32/winaspi16.c
Changelog:
- malloc calls in dlls/kernel32/process.c have been change to HeapAlloc
Your changelog (right above) doesn't match the patch, and
PMIB_IPADDRTABLE buf = (PMIB_IPADDRTABLE)malloc(dwSize);
PMIB_IPADDRTABLE buf = (PMIB_IPADDRTABLE)HeapAlloc(GetProcessHeap(), 0, dwSize);
You've introduced a space after opening parentheses which does not match the style of the rest of the file.
-- James Hawkins
On Wed, Oct 29, 2008 at 8:02 PM, Pete Myers peterdanielmyers@googlemail.com wrote:
It does match the style of the way that HeapAlloc is used elsewhere in the file, though not malloc admittedly. I wasn't sure which convention had precedent.
Please bottom-post when replying to a post on the wine lists. There is one test function that uses a space after opening parentheses, but it's the exception, not the rule.
I'm afraid that I don't understand how my changelog doesn't match the patch. I'm new here, can you help me out? There's obviously something I'm missing!
You had several 'changelog' entries in the email, but the last one was
Changelog: * malloc calls in dlls/kernel32/process.c have been change to HeapAlloc
which does not match the patch.
2008/10/30 James Hawkins truiken@gmail.com
On Wed, Oct 29, 2008 at 8:02 PM, Pete Myers peterdanielmyers@googlemail.com wrote:
It does match the style of the way that HeapAlloc is used elsewhere in the file, though not malloc admittedly. I wasn't sure which convention had precedent.
Please bottom-post when replying to a post on the wine lists. There is one test function that uses a space after opening parentheses, but it's the exception, not the rule.
Sure, I'll be more careful about both of those in future.
I'm afraid that I don't understand how my changelog doesn't match the patch. I'm new here, can you help me out? There's obviously something I'm missing!
You had several 'changelog' entries in the email, but the last one was
Changelog:
- malloc calls in dlls/kernel32/process.c have been change to HeapAlloc
which does not match the patch.
That was a dumb mistake, caused by copying and pasting from a different patch. Thanks for your help James, I'll try and fix the patch then resubmit.
Pete Myers
Hello Pete,
James Hawkins wrote:
On Wed, Oct 29, 2008 at 8:02 PM, Pete Myers peterdanielmyers@googlemail.com wrote:
It does match the style of the way that HeapAlloc is used elsewhere in the file, though not malloc admittedly. I wasn't sure which convention had precedent.
Please bottom-post when replying to a post on the wine lists. There is one test function that uses a space after opening parentheses, but it's the exception, not the rule.
I'm afraid that I don't understand how my changelog doesn't match the patch. I'm new here, can you help me out? There's obviously something I'm missing!
You had several 'changelog' entries in the email, but the last one was
Changelog:
- malloc calls in dlls/kernel32/process.c have been change to HeapAlloc
which does not match the patch.
In addition to what James said please split the patch up; one patch per module. A dll or program is a "module".
Also while you are at it please remove the superfluous casts in the lines you are changing. HeapAlloc (malloc too) returns a void pointer which doesn't needs to be casted to an other pointer type. E.g. --- a/dlls/iphlpapi/tests/iphlpapi.c +++ b/dlls/iphlpapi/tests/iphlpapi.c @@ -225,7 +225,7 @@ static void testGetIpAddrTable(void) "GetIpAddrTable(NULL, &dwSize, FALSE) returned %d, expected ERROR_INSUFFICIENT_BUFFER\n", apiReturn); if (apiReturn == ERROR_INSUFFICIENT_BUFFER) { - PMIB_IPADDRTABLE buf = (PMIB_IPADDRTABLE)malloc(dwSize); + PMIB_IPADDRTABLE buf = (PMIB_IPADDRTABLE)HeapAlloc( GetProcessHeap(), 0, dwSize);
apiReturn = gGetIpAddrTable(buf, &dwSize, FALSE); ok(apiReturn == NO_ERROR,
should really be just: PMIB_IPADDRTABLE buf = HeapAlloc(GetProcessHeap(), 0, dwSize);
thanks bye michael
In addition to what James said please split the patch up; one patch per module. A dll or program is a "module".
Sorry, I'm inundated with noobie errors here I think.
Also while you are at it please remove the superfluous casts in the lines you are changing. HeapAlloc (malloc too) returns a void pointer which doesn't needs to be casted to an other pointer type. E.g.
Will do. Didn't want to change coding style.
Actually my big struggle at the moment is trying to figure out where malloc() memory gets free(). What's the policy here? Should HeapAlloc() always be set free... Is the fact that some malloc() memory not free()'d an error, or a policy? Or should I work harder to find out where free() gets called?
Pete
Pete Myers wrote:
In addition to what James said please split the patch up; one patch per module. A dll or program is a "module".
Sorry, I'm inundated with noobie errors here I think.
No problem. Janitorial tasks are really good to accustom new people to the patch submission process.
Also while you are at it please remove the superfluous casts in the lines you are changing. HeapAlloc (malloc too) returns a void pointer which doesn't needs to be casted to an other pointer type. E.g.
Will do. Didn't want to change coding style.
Well that's not coding style. Superfluous casts are evil. I have even created a new janitorial tasks for that: http://wiki.winehq.org/SuperfluousCasts
Also if one has to touch a line it is expected from him to fix all errors in that line. That includes dangling whitespace at the end of the line and mixed tab and space indentation.
Actually my big struggle at the moment is trying to figure out where malloc() memory gets free(). What's the policy here? Should
Just search for free in that file.
HeapAlloc() always be set free... Is the fact that some malloc() memory not free()'d an error, or a policy? Or should I work harder to
You need to replace the free calls with HeapFree calls.
find out where free() gets called?
grep -w free dlls/iphlpapi/tests/iphlpapi.c dlls/wnaspi32/winaspi16.c That will do.
bye michael
-
James Hawkins -
Michael Stefaniuc -
Pete Myers