Re: MSI: indentation and style cleanup
Mike McCormack <mike(a)codeweavers.com> writes:
- if(szFilePath) { + if( szFilePath ) + { len = MultiByteToWideChar( CP_ACP, 0, szFilePath, -1, NULL, 0 ); szwFilePath = HeapAlloc( GetProcessHeap(), 0, len*sizeof(WCHAR) ); - if( !szwFilePath) - return ERROR_OUTOFMEMORY; MultiByteToWideChar( CP_ACP, 0, szFilePath, -1, szwFilePath, len ); }
Does your coding style also forbid proper error checking, or is there another reason for removing that check? <g> -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard wrote:
Does your coding style also forbid proper error checking, or is there another reason for removing that check? <g>
I thought that crashing on out of memory conditions was a valid method of error handling in the Julliard handbook of coding, so I removed a pointless check :) If we're going to mandate proper checking of memory allocation, then we need to make a janitorial task to do so, as there's many, many places in the code it's not done. Mike
Mike McCormack <mike(a)codeweavers.com> writes:
I thought that crashing on out of memory conditions was a valid method of error handling in the Julliard handbook of coding, so I removed a pointless check :)
If we're going to mandate proper checking of memory allocation, then we need to make a janitorial task to do so, as there's many, many places in the code it's not done.
Well yes, ultimately we will need to have proper checks everywhere. It's not really high priority, and I don't think there's a need to make an effort in this direction at this point. But at least we can avoid removing the checks that are already in place... -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard wrote:
Mike McCormack <mike(a)codeweavers.com> writes:
I thought that crashing on out of memory conditions was a valid method of error handling in the Julliard handbook of coding, so I removed a pointless check :)
If we're going to mandate proper checking of memory allocation, then we need to make a janitorial task to do so, as there's many, many places in the code it's not done.
Well yes, ultimately we will need to have proper checks everywhere. It's not really high priority, and I don't think there's a need to make an effort in this direction at this point. But at least we can Well, it should be pretty easy to write a short smatch script to find that occurences. Afair the smatch guys wrote one for the Linux kernel which would need only small adaptations. And coincidently i have this weekend a long flight trip on my schedule ... . And we seem to have some people now doing janitorial work. I figure 2-3 people would maybe jump on it, maybe even new people as the task isn't hard at all. If we get new people it's nice cause they get to see a lot of wine code and get used to the patch acceptance policy ;). Maybe they get motivated to try there luck on other wine tasks too.
avoid removing the checks that are already in place...
bye michael -- Michael Stefaniuc Tel.: +49-711-96437-199 System Administration Fax.: +49-711-96437-111 Red Hat GmbH Email: mstefani(a)redhat.com Hauptstaetterstr. 58 http://www.redhat.de/ D-70178 Stuttgart
On Fri, 21 Jan 2005 22:02:00 +0100, Michael Stefaniuc wrote:
Well, it should be pretty easy to write a short smatch script to find that occurences. Afair the smatch guys wrote one for the Linux kernel which would need only small adaptations.
OOM safety is a bit complicated, you have to properly unwind the stack and restore state as you go - for instance the last patch I submitted fixed a bug where OOM would not cause the loop to terminate, but I forgot to free some data as we returned up the stack. Given that it can be quite complex and introduce new bugs, and given that it's really quite a useless feature IMHO as modern Linux boxes will hang themselves in swap hell before returning NULL from malloc I don't think this should be a janitorial project. Just my humble opinion, of course ;) thanks -mike
Mike Hearn wrote:
On Fri, 21 Jan 2005 22:02:00 +0100, Michael Stefaniuc wrote:
Well, it should be pretty easy to write a short smatch script to find that occurences. Afair the smatch guys wrote one for the Linux kernel which would need only small adaptations.
OOM safety is a bit complicated, you have to properly unwind the stack and restore state as you go - for instance the last patch I submitted fixed a bug where OOM would not cause the loop to terminate, but I forgot to free some data as we returned up the stack.
Given that it can be quite complex and introduce new bugs, and given that it's really quite a useless feature IMHO as modern Linux boxes will hang themselves in swap hell before returning NULL from malloc I don't think this should be a janitorial project. I thought more of checking the return value of HeapAlloc/HeapRealloc to make sure it's not NULL. This would be easy to do. What you proposed is too complicated.
bye michael -- Michael Stefaniuc Tel.: +49-711-96437-199 System Administration Fax.: +49-711-96437-111 Red Hat GmbH Email: mstefani(a)redhat.com Hauptstaetterstr. 58 http://www.redhat.de/ D-70178 Stuttgart
On Sun, 23 Jan 2005 10:33:38 +0100, Michael Stefaniuc wrote:
I thought more of checking the return value of HeapAlloc/HeapRealloc to make sure it's not NULL. This would be easy to do. What you proposed is too complicated.
Well, what happens if it is NULL? You can print an error and then return ... but then the code looks OOM safe even though it's actually not. I guess that's similar to my thoughts on the Interlocked changes before though, it makes code that isn't thread safe look like it is. thanks -mike
On Sat, Jan 22, 2005 at 05:16:45PM +0000, Mike Hearn wrote:
Given that it can be quite complex and introduce new bugs, and given that it's really quite a useless feature IMHO as modern Linux boxes will hang themselves in swap hell before returning NULL from malloc I don't think this should be a janitorial project.
I suspect that some of the cases will be simple, some a bit more complicated, a few quite involved. However, it's not that difficult to 'get' what you need to do, so if someone picks up the task, they'll get it within a few patches. ANd they can start with the simple stuff, BTW. At the very list, I think it's worth having the script. And while we're at it, we might as well have the list, for curiosity's sake. :) -- Dimi.
Mike Hearn wrote:
OOM safety is a bit complicated, you have to properly unwind the stack and restore state as you go - for instance the last patch I submitted fixed a bug where OOM would not cause the loop to terminate, but I forgot to free some data as we returned up the stack.
Given that it can be quite complex and introduce new bugs, and given that it's really quite a useless feature IMHO as modern Linux boxes will hang themselves in swap hell before returning NULL from malloc I don't think this should be a janitorial project.
You can get NULL with a corrupted heap too. Rob
Michael Stefaniuc <mstefani(a)redhat.com> writes:
Well, it should be pretty easy to write a short smatch script to find that occurences. Afair the smatch guys wrote one for the Linux kernel which would need only small adaptations. And coincidently i have this weekend a long flight trip on my schedule ... . And we seem to have some people now doing janitorial work. I figure 2-3 people would maybe jump on it, maybe even new people as the task isn't hard at all. If we get new people it's nice cause they get to see a lot of wine code and get used to the patch acceptance policy ;). Maybe they get motivated to try there luck on other wine tasks too.
Yes, but please let's wait until some of the current janitorial tasks are finished before starting new ones. We don't want to have real changes drowned out by a ton of janitorial changes all over the code base. -- Alexandre Julliard julliard(a)winehq.org
participants (6)
-
Alexandre Julliard -
Dimitrie O. Paun -
Michael Stefaniuc -
Mike Hearn -
Mike McCormack -
Rob Shearman