Re: cabinet: Add breaks to switch statements

[Dan Kegel]

On 9/13/06, Andrew Talbot <Andrew.Talbot(a)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

cabinet: Add breaks to switch statements

[Dimi Paun]

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 <dimi(a)lattica.com>
Lattica, Inc.

cabinet: Add breaks to switch statements

[Andrew Talbot]

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.