Re: Improve handling of invalid input in dlls/comctl32/datetime.c
On 12/25/2009 14:18, Gerald Pfeifer wrote:
Otherwise max_count will be undefined in the following loop may do interesting things it seems. (Does Coverity diagnose similar items?)
Gerald
ChangeLog: Improve handling of invalid input in DATETIME_ReturnFieldWidth.
diff --git a/dlls/comctl32/datetime.c b/dlls/comctl32/datetime.c index c240d4f..08c7082 100644 --- a/dlls/comctl32/datetime.c +++ b/dlls/comctl32/datetime.c @@ -38,6 +38,7 @@ * -- FORMATCALLBACK */
+#include<assert.h> #include<math.h> #include<string.h> #include<stdarg.h> @@ -619,6 +620,9 @@ DATETIME_ReturnFieldWidth (const DATETIME_INFO *infoPtr, HDC hdc, int count, SHO lctype = LOCALE_SMONTHNAME1; max_count = 12; break; + default: + assert(0); + max_count=0; }
cx = 0;
Hi, Gerald. This can't even happen. spec if filtered later with: --- case THREECHARMONTH: case FULLMONTH: case THREECHARDAY: case FULLDAY: --- So no default case here.
Nikolay Sivov wrote:
On 12/25/2009 14:18, Gerald Pfeifer wrote:
Otherwise max_count will be undefined in the following loop may do interesting things it seems. (Does Coverity diagnose similar items?)
Gerald
ChangeLog: Improve handling of invalid input in DATETIME_ReturnFieldWidth.
diff --git a/dlls/comctl32/datetime.c b/dlls/comctl32/datetime.c index c240d4f..08c7082 100644 --- a/dlls/comctl32/datetime.c +++ b/dlls/comctl32/datetime.c @@ -38,6 +38,7 @@ * -- FORMATCALLBACK */
+#include<assert.h> #include<math.h> #include<string.h> #include<stdarg.h> @@ -619,6 +620,9 @@ DATETIME_ReturnFieldWidth (const DATETIME_INFO *infoPtr, HDC hdc, int count, SHO lctype = LOCALE_SMONTHNAME1; max_count = 12; break; + default: + assert(0); + max_count=0; }
cx = 0;
Hi, Gerald.
This can't even happen. spec if filtered later with: --- case THREECHARMONTH: case FULLMONTH: case THREECHARDAY: case FULLDAY: --- So no default case here.
Even if the default case cannot be reached, it is not a 'bad thing' to have one, even if it is an error message stating "Something went wrong, we should not get here" with an exit code... James McKenzie
On 12/26/2009 00:04, James McKenzie wrote:
Nikolay Sivov wrote:
On 12/25/2009 14:18, Gerald Pfeifer wrote:
Otherwise max_count will be undefined in the following loop may do interesting things it seems. (Does Coverity diagnose similar items?)
Gerald
ChangeLog: Improve handling of invalid input in DATETIME_ReturnFieldWidth.
diff --git a/dlls/comctl32/datetime.c b/dlls/comctl32/datetime.c index c240d4f..08c7082 100644 --- a/dlls/comctl32/datetime.c +++ b/dlls/comctl32/datetime.c @@ -38,6 +38,7 @@ * -- FORMATCALLBACK */
+#include<assert.h> #include<math.h> #include<string.h> #include<stdarg.h> @@ -619,6 +620,9 @@ DATETIME_ReturnFieldWidth (const DATETIME_INFO *infoPtr, HDC hdc, int count, SHO lctype = LOCALE_SMONTHNAME1; max_count = 12; break; + default: + assert(0); + max_count=0; }
cx = 0;
Hi, Gerald.
This can't even happen. spec if filtered later with: --- case THREECHARMONTH: case FULLMONTH: case THREECHARDAY: case FULLDAY: --- So no default case here.
Even if the default case cannot be reached, it is not a 'bad thing' to have one, even if it is an error message stating "Something went wrong, we should not get here" with an exit code...
Why? There's no default case, treat is as 'if () {} else if () {} etc.'. It's the same thing to have explicit initializers for all local variables even if I don't use it before set some value. It's a pollution, especially if used to silence analyzer warnings.
James McKenzie
Nikolay Sivov wrote:
On 12/26/2009 00:04, James McKenzie wrote:
Nikolay Sivov wrote:
On 12/25/2009 14:18, Gerald Pfeifer wrote:
Otherwise max_count will be undefined in the following loop may do interesting things it seems. (Does Coverity diagnose similar items?)
Gerald
ChangeLog: Improve handling of invalid input in DATETIME_ReturnFieldWidth.
diff --git a/dlls/comctl32/datetime.c b/dlls/comctl32/datetime.c index c240d4f..08c7082 100644 --- a/dlls/comctl32/datetime.c +++ b/dlls/comctl32/datetime.c @@ -38,6 +38,7 @@ * -- FORMATCALLBACK */
+#include<assert.h> #include<math.h> #include<string.h> #include<stdarg.h> @@ -619,6 +620,9 @@ DATETIME_ReturnFieldWidth (const DATETIME_INFO *infoPtr, HDC hdc, int count, SHO lctype = LOCALE_SMONTHNAME1; max_count = 12; break; + default: + assert(0); + max_count=0; }
cx = 0;
Hi, Gerald.
This can't even happen. spec if filtered later with: --- case THREECHARMONTH: case FULLMONTH: case THREECHARDAY: case FULLDAY: --- So no default case here.
Even if the default case cannot be reached, it is not a 'bad thing' to have one, even if it is an error message stating "Something went wrong, we should not get here" with an exit code...
Why? There's no default case, treat is as 'if () {} else if () {} etc.'. It's the same thing to have explicit initializers for all local variables even if I don't use it before set some value. It's a pollution, especially if used to silence analyzer warnings. Nikolay:
Things can and do go 'wrong'. For instance, you build on Linux, I build on a Mac. Sometimes the ported programs from UNIX can and will do 'strange' things. If you don't have a method to know that there is an error, then you go about blaming Wine when it is a poor program port. Also, it is good programming practice to account for the situations you don't expect to encounter and to initialize variables that you will be using. This is not a 'just in case' thing, it can help when program errors occur to see where in the code an expected value did or did not happen. I know, it looks like pollution, but when code goes wrong, and it will, this makes troubleshooting much easier. I know this from running Quality Assurance testing for many years and coding myself. My programming instructor deducted grade points for failing to handle error and other unexpected conditions. James McKenzie
On 12/26/2009 00:47, James McKenzie wrote:
Nikolay Sivov wrote:
Why? There's no default case, treat is as 'if () {} else if () {} etc.'. It's the same thing to have explicit initializers for all local variables even if I don't use it before set some value. It's a pollution, especially if used to silence analyzer warnings.
Nikolay:
Things can and do go 'wrong'. For instance, you build on Linux, I build on a Mac. Sometimes the ported programs from UNIX can and will do 'strange' things. If you don't have a method to know that there is an error, then you go about blaming Wine when it is a poor program port.
That's a Mac build environment then. If there's no real problem now, there's no point to add such workarounds, if it is then a compiler should be fixed.
Also, it is good programming practice to account for the situations you don't expect to encounter and to initialize variables that you will be using. This is not a 'just in case' thing, it can help when program errors occur to see where in the code an expected value did or did not happen.
It also hides things sometimes and adds noops.
I know, it looks like pollution, but when code goes wrong, and it will, this makes troubleshooting much easier. It will go wrong only due a build problem or something like that, it such case you can't trust a line. I know this from running Quality Assurance testing for many years and coding myself. My programming instructor deducted grade points for failing to handle error and other unexpected conditions.
Ok, in theory yes, but I'm speaking about a particular piece.
James McKenzie
Anyway, my point is that code in subject is clear and can't cause problems patch tries to predict - it's the same condition used twice. I really have nothing to add here, let's see Alexandre's opinion.
Nikolay Sivov wrote:
On 12/26/2009 00:47, James McKenzie wrote:
Nikolay Sivov wrote:
Why? There's no default case, treat is as 'if () {} else if () {} etc.'. It's the same thing to have explicit initializers for all local variables even if I don't use it before set some value. It's a pollution, especially if used to silence analyzer warnings.
Nikolay:
Things can and do go 'wrong'. For instance, you build on Linux, I build on a Mac. Sometimes the ported programs from UNIX can and will do 'strange' things. If you don't have a method to know that there is an error, then you go about blaming Wine when it is a poor program port.
That's a Mac build environment then. If there's no real problem now, there's no point to add such workarounds, if it is then a compiler should be fixed. With this statement, I agree. I am dealing with a sed issue that causes the authors.c file in the Wine build to break. Is this Wine's fault? No. It is a problem outside of the scope of the project. Would additional code fix this in Wine, no.
Also, it is good programming practice to account for the situations you don't expect to encounter and to initialize variables that you will be using. This is not a 'just in case' thing, it can help when program errors occur to see where in the code an expected value did or did not happen.
It also hides things sometimes and adds noops. Agreed. But if there is code we should NEVER reach, maybe it is good to tell someone. That is why I like defaults in cases where the other values should be the only values reached. If you are doing checks before the call that invokes the case, then there is no need for the default case as erroneous data is already checked for. I've used that as well.
I know, it looks like pollution, but when code goes wrong, and it will, this makes troubleshooting much easier. It will go wrong only due a build problem or something like that, it such case you can't trust a line. I trust NOTHING. That includes this computer. It can and has failed on me. However, you can do things, like tested backups that can mitigate failures... I know this from running Quality Assurance testing for many years and coding myself. My programming instructor deducted grade points for failing to handle error and other unexpected conditions.
Ok, in theory yes, but I'm speaking about a particular piece. Thank you. Anyway, my point is that code in subject is clear and can't cause problems patch tries to predict - it's the same condition used twice.
I really have nothing to add here, let's see Alexandre's opinion.
Yes, AJs opinion is the 'last word' on code entering Wine. I've seen code that I would not pass get into the project base and code I would have passed get rejected. AJs long history with this project gives him knowledge that some of us don't and will not have. I agree that we should see what AJ has to say about this particular section of code. James McKenzie
For the record, this is how this has been addressed a few months after this discussion. That's another way of doing it. :-) Gerald commit 1af17844301c4924dd8324cbf2f9c3c1ef0394b2 Author: Huw Davies <huw(a)codeweavers.com> Date: Tue May 4 14:05:13 2010 +0100 comctl32: Silence a few compiler warnings. diff --git a/dlls/comctl32/datetime.c b/dlls/comctl32/datetime.c index dc2dd12..2418950 100644 --- a/dlls/comctl32/datetime.c +++ b/dlls/comctl32/datetime.c @@ -614,7 +614,7 @@ DATETIME_ReturnFieldWidth (const DATETIME_INFO *infoPtr, HDC hdc, int count, SHO lctype = LOCALE_SABBREVMONTHNAME1; max_count = 12; break; - case FULLMONTH: + default: /* FULLMONTH */ fall = fld_mon; lctype = LOCALE_SMONTHNAME1; max_count = 12; On Fri, 25 Dec 2009, James McKenzie wrote:
Nikolay Sivov wrote:
On 12/26/2009 00:04, James McKenzie wrote:
Nikolay Sivov wrote:
On 12/25/2009 14:18, Gerald Pfeifer wrote:
Otherwise max_count will be undefined in the following loop may do interesting things it seems. (Does Coverity diagnose similar items?)
Gerald
ChangeLog: Improve handling of invalid input in DATETIME_ReturnFieldWidth.
diff --git a/dlls/comctl32/datetime.c b/dlls/comctl32/datetime.c index c240d4f..08c7082 100644 --- a/dlls/comctl32/datetime.c +++ b/dlls/comctl32/datetime.c @@ -38,6 +38,7 @@ * -- FORMATCALLBACK */
+#include<assert.h> #include<math.h> #include<string.h> #include<stdarg.h> @@ -619,6 +620,9 @@ DATETIME_ReturnFieldWidth (const DATETIME_INFO *infoPtr, HDC hdc, int count, SHO lctype = LOCALE_SMONTHNAME1; max_count = 12; break; + default: + assert(0); + max_count=0; }
cx = 0;
Hi, Gerald.
This can't even happen. spec if filtered later with: --- case THREECHARMONTH: case FULLMONTH: case THREECHARDAY: case FULLDAY: --- So no default case here.
Even if the default case cannot be reached, it is not a 'bad thing' to have one, even if it is an error message stating "Something went wrong, we should not get here" with an exit code...
Why? There's no default case, treat is as 'if () {} else if () {} etc.'. It's the same thing to have explicit initializers for all local variables even if I don't use it before set some value. It's a pollution, especially if used to silence analyzer warnings. Nikolay:
Things can and do go 'wrong'. For instance, you build on Linux, I build on a Mac. Sometimes the ported programs from UNIX can and will do 'strange' things. If you don't have a method to know that there is an error, then you go about blaming Wine when it is a poor program port. Also, it is good programming practice to account for the situations you don't expect to encounter and to initialize variables that you will be using. This is not a 'just in case' thing, it can help when program errors occur to see where in the code an expected value did or did not happen.
I know, it looks like pollution, but when code goes wrong, and it will, this makes troubleshooting much easier. I know this from running Quality Assurance testing for many years and coding myself. My programming instructor deducted grade points for failing to handle error and other unexpected conditions.
James McKenzie
participants (3)
-
Gerald Pfeifer -
James McKenzie -
Nikolay Sivov