Hi,
After seeing Gerald Pfeifer's patch, I decided to see how many asserts were used in wine.
$ find . -type f -iname *.c -exec grep -H assert '{}' ; > assert1.log
This returns 2313 matching lines, some of which are including <assert.h>.
$ cat assert1.log | sed -e 's/:.*//' | sort | uniq -c | sort -n > assert2.log
This shows that there are 454 files that make use of assert, of which 166 files only reference assert once.
$ cat assert2.log | grep " 1 " | sed -e 's/ *1 //' | xargs grep -H "#include <assert.h>" > assert3.log
This shows that 151 files pull in assert.h but don't reference it in the c file, so could be removed.
NOTE: The results are out by a few files as some of the results just have assert in a comment (such as dlls/wined3d/arb_program_shader.c).
-----
As a general principle, assert in: 1/ tests should be changed to an ok and possibly a skip -- otherwise, the test results are reported as a crash, making it harder to find out where the problem is. 2/ dlls should be changed to a SetLastError or failure return code (depending on the API) -- otherwise, the program triggering this will be killed with no information as to what happened. 3/ tools should print an error message indicating the problem (failed to allocate memory, could not open file, ...) preferably with as much information as possible (file does not exist, user does not have privileges to open/write to file, ...) -- this is to make it easier to see what the problem is and correct it. 4/ server -- not sure here, but this should ideally return an error code to the caller (APIs?) so it can be handled by that API/program. 5/ programs should display an error to the user (e.g. MessageBox) indicating the nature of the problem (file does not exist, ...) in a way that is understandable by the user and (if possible) recover from the error, or otherwise inform the user that the application cannot recover (e.g. out of memory).
- Reece
On 03/07/2010 04:38 AM, Reece Dunn wrote:
As a general principle, assert in: 1/ tests should be changed to an ok and possibly a skip -- otherwise, the test results are reported as a crash, making it harder to find out where the problem is.
+1 In most cases they check for something that should never happen (creating window fails). So using skip is appropriate.
2/ dlls should be changed to a SetLastError or failure return code (depending on the API) -- otherwise, the program triggering this will be killed with no information as to what happened.
This depends. If something catastrophic happened and returning to application will surely cause it's crash - then it's better assert inside Wine. You'll have much better traces and information about the source of the problem. But if an app just passed invalid parameters returning error would seem to be appropriate.
3/ tools should print an error message indicating the problem (failed to allocate memory, could not open file, ...) preferably with as much information as possible (file does not exist, user does not have privileges to open/write to file, ...) -- this is to make it easier to see what the problem is and correct it.
Using asserts in our tools is the most appropriate place. Of course it doesn't mean one can avoid proper error handling. So it's it's case by case.
4/ server -- not sure here, but this should ideally return an error code to the caller (APIs?) so it can be handled by that API/program.
No, asserts in server are guards against codding errors. They check things that should never ever happen inside server. If they do - it's a programmer's error, not application's.
This is so whomever touches server have a good chance to catch and fix any missed problems.
5/ programs should display an error to the user (e.g. MessageBox) indicating the nature of the problem (file does not exist, ...) in a way that is understandable by the user and (if possible) recover from the error, or otherwise inform the user that the application cannot recover (e.g. out of memory).
Pretty much the same as tools - asserts are bad replacement for proper error checking. But fine to prevent major damage.
Vitaliy.
On 03/07/2010 10:35 AM, Vitaliy Margolen wrote:
On 03/07/2010 04:38 AM, Reece Dunn wrote:
2/ dlls should be changed to a SetLastError or failure return code (depending on the API) -- otherwise, the program triggering this will be killed with no information as to what happened.
This depends. If something catastrophic happened and returning to application will surely cause it's crash - then it's better assert inside Wine. You'll have much better traces and information about the source of the problem. But if an app just passed invalid parameters returning error would seem to be appropriate.
If we're trying to mimic Windows behavior, isn't a BSOD more appropriate? ;-)
Dan
-
Dan McDonald -
Reece Dunn -
Vitaliy Margolen