On 9/13/06, Andrew Talbot Andrew.Talbot@talbotville.com wrote:
On Wednesday 13 September 2006 04:22, you wrote:
Does adding the breaks fix conformance tests?
No, I haven't fixed anything with this patch: it was a neutral one, in that respect. I just put it in because I perceived that those switch statements were not relying on any fallthrough, so I thought it good practice to isolate the cases.
Let's see: your change was
--- a/dlls/cabinet/fci.c 2006-09-12 11:55:48.000000000 +0100 +++ b/dlls/cabinet/fci.c 2006-09-12 23:29:02.000000000 +0100 @@ -454,11 +454,12 @@ switch (cb % 4) { case 3: ul |= (((ULONG)(*pb++)) << 16); + break; case 2: ul |= (((ULONG)(*pb++)) << 8); + break; case 1: ul |= *pb++; - default: break; }
Offhand that looks like a more serious change than you thought. You'd better either really understand the code, or add a conformance test that shows this fixes a bug, IMHO. - Dan
On Wed, September 13, 2006 1:18 pm, Dan Kegel wrote:
Offhand that looks like a more serious change than you thought. You'd better either really understand the code, or add a conformance test that shows this fixes a bug, IMHO.
Agreed. But that is confusing code, it should have a /* fallthrough */ comment instead of the break statement if that was the intent.
Dimi Paun wrote:
On Wed, September 13, 2006 1:18 pm, Dan Kegel wrote:
Offhand that looks like a more serious change than you thought. You'd better either really understand the code, or add a conformance test that shows this fixes a bug, IMHO.
Agreed. But that is confusing code, it should have a /* fallthrough */ comment instead of the break statement if that was the intent.
And I shall humbly learn to properly check how code works before "fixing" it, in future. :) Thanks, Dan and Dimi, for your helpful comments.
-- Andy.
-
Andrew Talbot -
Dan Kegel -
Dimi Paun