Switch me harder



  • Found in a real open-source app.  Can't decide between switch/case and if/else? Use both!
    	switch (asys->clip_oversamples) {
    case 1:
    if (temp > 255)
    *buf++ = 255;
    else if (temp < 0)
    *buf++ = 0;
    else
    case 0:
    *buf++ = temp;
    }


  • This guy must have seen Duff's Device and thought, "Hmm... I guess it's common practice..." 

    http://en.wikipedia.org/wiki/Duff's_device

     



  • Um... tell me how exactly you'd combine those two into a single switch statement. I mean, if you had something like

    switch (asys->clip_oversamples){
        case 1:
        case 2:
        case 3:
            if (asys->clip_oversamples < 2)
                /* ... */
            else
                /* ... */
        case 0:
            /* ... */
    }
     

    I could understand you, but in the example above, both expressions seem to be totally unrelated. The if block couldn't be converted to a switch block at all, and, due to the missing break (at least if its intentional), converting the switch block into a second if block wouldn't be that easy either.

    I agree, this looks weird on first glance, but I think this is propably a good solution here if you want to keep stuff easy and readable. 



  • [quote user="PSWorx"]

    ...and readable.

    [/quote]

    Readable ??!? particularly the missing break; makes it very unreadable indeed.

    How about this?

     if (asys->clip_oversamples == 1)
     {
       if (temp > 255)
          *buf++ = 255;
      else if (temp < 0)
          *buf++ = 0;
      else
          *buf++ = temp;
      }
      else if (asys->clip_oversamples == 0)
      {
        *buf++ = temp;
      }

    I know - it duplicates one line of code, but is otherwise at least ten times as readable.



  • Easy and readable my ass.  Did anyone notice that the second else leads to fallthrough?

     

    Non-WTF rewrite:

     

    #define MAX(a,b) ( (a) > (b) ? (a) : (b) )

    #define MIN(a,b) ( (a) < (b) ? (a) : (b) )

     

    switch (asys->clip_oversamples) {
      case 1:
        *buf++ = MAX(MIN(temp,0),255);
        break;
      case 0:
        *buf++ = temp;
        break;
    }



  • [quote user="emurphy"]

    Easy and readable my ass.  Did anyone notice that the second else leads to fallthrough?

     

    Non-WTF rewrite:

     

    #define MAX(a,b) ( (a) > (b) ? (a) : (b) )

    #define MIN(a,b) ( (a) < (b) ? (a) : (b) )

     

    switch (asys->clip_oversamples) {
      case 1:
        *buf++ = MAX(MIN(temp,0),255);
        break;
      case 0:
        buf++ = temp;
        break;
    }

    [/quote]

    The original is typical, for C code.  It would be good to add a / fall through */ comment (and required to pass through PC-LINT). 

    Also it's a lot easier to write it correctly the original way.  Try the quoted rewrite with asys->clip_oversamples = 1, temp = 100.

     



  • You mixed your MIN and MAX up.  It should be: MIN(MAX(temp,0),255);



  • [quote user="Licky Lindsay"]

    Found in a real open-source app.  Can't decide between switch/case and if/else? Use both!
    	switch (asys->clip_oversamples) {
    case 1:
    if (temp > 255)
    *buf++ = 255;
    else if (temp < 0)
    *buf++ = 0;
    else
    case 0:
    *buf++ = temp;
    }

    [/quote]

    What appears to be happening here is really neat, and i may actually use this. He wants case 0 to always run, but he wants to perform some housekeeping if (asys->clip_oversamples) is 1. You could do this with if/then/elseif, but you wouldn't gain much in readability, i wouldn't think.

    if ( (asys->clip_oversamples) && ( (temp > 255) || (temp < 0) ) )

         if (temp>255)

             *buf++ = 255

         else

     



  • [quote user="Licky Lindsay"]

    Found in a real open-source app.  Can't decide between switch/case and if/else? Use both!
    	switch (asys->clip_oversamples) {
    case 1:
    if (temp > 255)
    *buf++ = 255;
    else if (temp < 0)
    *buf++ = 0;
    else
    case 0:
    *buf++ = temp;
    }

    [/quote]

    What appears to be happening here is really neat, and i may actually use this. He wants case 0 to always run, but he wants to perform some housekeeping if (asys->clip_oversamples) is 1. You could do this with if/then/elseif, but you wouldn't gain much in readability, i wouldn't think.

    if ( (asys->clip_oversamples) && ( (temp > 255) || (temp < 0) ) )

         if (temp>255)

             *buf++ = 255

         else

     

     



  • [quote user="Licky Lindsay"]

    Found in a real open-source app.  Can't decide between switch/case and if/else? Use both!
    	switch (asys->clip_oversamples) {
    case 1:
    if (temp > 255)
    *buf++ = 255;
    else if (temp < 0)
    *buf++ = 0;
    else
    case 0:
    *buf++ = temp;
    }

    [/quote]

    What appears to be happening here is really neat, and i may actually use this. He wants case 0 to always run, but he wants to perform some housekeeping if (asys->clip_oversamples) is 1. You could do this with if/then/elseif, but you wouldn't gain much in readability, i wouldn't think.

    if ( (asys->clip_oversamples) && ( (temp > 255) || (temp < 0) ) )

    Ok i just reread the code snippet for the fourth time, and i realized that Case 1 doesn't break, making it irrelevant. IF, on the other hand *buf++ = 255 was temp = 255 instead, then this would make sense. yah this code snippet is messed up. why even have the first case statement chance something that's going to get changed again? are there any languages that autobreak between case statements? i am really scratching my head at why i spent so much time trying to find a better way to do this, when obviously without knowing wtf this is doing i can't even begin.

     



  • I would imagine that this is a bug and that the else should be a break. I doubt the developer wanted to increment the buf pointer twice in those special cases.



  • [quote user="welcor"][quote user="PSWorx"]

    ...and readable.

    [/quote]

    Readable ??!? particularly the missing break; makes it very unreadable indeed.

    How about this?

     if (asys->clip_oversamples == 1)
     {
       if (temp > 255)
          *buf++ = 255;
      else if (temp < 0)
          *buf++ = 0;
      else
          *buf++ = temp;
      }
      else if (asys->clip_oversamples == 0)
      {
        *buf++ = temp;
      }

    I know - it duplicates one line of code, but is otherwise at least ten times as readable.

    [/quote]

    Not too good, actually.  From context, asys->clip_oversamples is a boolean test, so the test for 0 is redundant.  Either the value is 1 or 0; if it ain't 1, then it's 0--no need for the extra test.  The combined switch/if construct is perfectly legal C/C++, and probably even generates decent machine code when compiled.  Those of you that aren't comfortable with letting cases fall through--and even seem to think that there's an implementation fault with this--just haven't been coding long enough, or believe too much of what your Java teachers are selling.

    Those of you who have only programmed at the C/C++ level and don't know assembly language don't realize that a decent compiler does switch() compilations as jump tables.  That's vastly more efficient than the ladder of cmp (type) instructions the compiler has to generate otherwise;  those that think the MIN/MAX macros are going to help the situation are wrong, because the compiler output of those will be a sorry mess.  The ?: stuff looks cute in code, but generally produces worse assembly than a regular if/then/else.  Seriously.

    As written, good.  As amended...well, sad.

    So say I, after 30 years of programming.



  • I'm curious about efficient implementations of the switch() statement. I know that it's can be translated using an indexed jump and a jump table, but that requires that the cases are all very close together, preferably without any gaps to make it efficient.

    So given a switch(X) with cases 2,3,4,5,6,7,8,9, yes you can just subtract 2 from X and then use that as the offset to your jump table and find the right part of code that way. But what do you do with "real" data? Even in the compiler design lectures, the jump table idea was only used as a first idea.

    I decided to see what some real compilers do, and wrote a switching function in java and C++ (using g++).

    public static int s(int i){
    switch(i)
    {
    case 2:
    return 2;

    case 300:
    return i*i;

    case -5:
    System.out.println("-5");

    case 1<<62:
    return 0;

    default:
    throw new RuntimeException();
    }
    }
    ahe output of javap -c
    Code:
    0: iload_0
    1: lookupswitch{ //4
    -5: 50;
    2: 44;
    300: 46;
    1073741824: 58;
    default: 60 }
    44: iconst_2
    45: ireturn
    46: iload_0
    47: iload_0
    48: imul
    49: ireturn
    50: getstatic #17; //Field java/lang/System.out:Ljava/io/PrintStream;
    53: ldc #45; //String -5
    55: invokevirtual #47; //Method java/io/PrintStream.println:(Ljava/lang/String;)V
    58: iconst_0
    59: ireturn
    60: new #51; //class java/lang/RuntimeException
    63: dup
    64: invokespecial #53; //Method java/lang/RuntimeException."":()V
    67: athrow

    From the "Java Virtual Machine" reference I gather that "[lookupswitch] is used to perform an efficient compare-and-jump, as might be needed for a switch statement. The table used by lookupswitch is given after the lookupswitch opcode in bytecode."

    The GNU C++ compiler does the same thing..

    int s(int i){
    switch(i)
    {
    case 2:
    return 2;

    case 300:
    return i*i;

    case -5:
    cout << i << endl;

    case 3<<30:
    return 0;

    default:
    cerr << "error" << endl;
    }
    }

    and the assembly for that:
        cmpl    $-5, %eax
    je L30
    jg L43
    cmpl $-1073741824, %eax
    je L40
    L41:

    ....

    L43:
    cmpl $2, %eax
    je L27
    cmpl $300, %eax
    jne L4

    and so on. Of course, I would've been very surprised if jump tables had been used. With such a large range in values, they would've used many gigabyte of memory :)

    What I want to know now: are jump tables really used anywhere in "decent compilers?" Surely not as the special case, but are they even worth the bother to add them as a optimization for small (in range) switch functions?



  • [quote user="RShilling"]You mixed your MIN and MAX up.  It should be: MIN(MAX(temp,0),255);[/quote]

     

    Whoops, of course you're right.  Still, any sane unit test would've detected that bug in a heartbeat.

     



  • Jump tables are used in "decent" compilers where it makes sense.  Contiguous values will often be in a jump table.  A single switch might be broken into multiple jump tables.

     

    If/then type constructs will be used where they take less space... primarily in situations where there are discontinuous values or fewer than a threshhold number of cases.  Depending on the compiler, that threshhold can be different, generally in the range of 5 or 7 or so.

     



  • Sure, this is "neat". So is Duff's Device (as mentioned above), that doesn't mean you should use it – fallthrough switches are a cunning use of C, but they're uncomfortably similar to computed GOTOs, and so can be desperately confusing. Using a case to jump inside a conditional block is doubly GOTO-esque.



    Saying it's more efficient than if/else is just premature optimisation.



    Also, using case forces you to assume that 1 is the only true value that asys->clip_oversamples is gonna take. I doubt that's guaranteed by whatever library this is from.



    I say stick with the obvious implementation:


    if (asys->clip_oversamples) {
    	if (temp > 255)
    		*buf++ = 255;
    	else if (temp < 0)
    		*buf++ = 0;
    	else
    		*buf++ = temp;
    }
    else
    	*buf++ = temp;


  • [quote user="Licky Lindsay"]

    Found in a real open-source app.  Can't decide between switch/case and if/else? Use both!
    	switch (asys->clip_oversamples) {
    case 1:
    if (temp > 255)
    *buf++ = 255;
    else if (temp < 0)
    *buf++ = 0;
    else
    case 0:
    *buf++ = temp;
    }

    [/quote]

     This way makes it fairly clear what's going on and why. 

     

        *buf++ = (asys->clip_oversamples == 1)

    	? ((temp < 0) ? 0 : (temp > 255 ? 255 : temp))
    	: temp;
     
      Of course the real wtf is that this construct is probably in the middle of a big processing loop 
    and asys->clip_oversamples is tested every single time through the loop and it should really
    be broken out into two loops, one clipping, one non-clipping.



  • I'm surprised noone has pointed out the obvious solution yet:

     

    if (asys->clip_oversamples)
      {
      if (temp>255) temp=255;else if (temp<0) temp=0;
      }
    *buf++ = temp; 

     



  • GeneWitch/tster: There's no bug. Putting aside efficiency concerns (I suspect DaveK is right here) and the fact the code looks a bit odd, it does do what it's supposed to.

    How come?

    The switch/case construct in C works like a bunch of conditional goto's. The original code (ignoring the case where clip is neither 0 nor 1) is equivalent to the following:

    if (asys->clip_oversamples == 0) {
    goto case0;
    }
    if (temp > 255) {
    *buf++ = 255;
    } else if (temp < 0) {
    *buf++ = 0;
    } else {
    case0:
    *buf++ = temp;
    }

    So in the case that asys->clip_oversamples == 1, the entire if statement is run normally, *buf will be set to one of {255, 0, temp} and then buf will be incremented.

    In the case that asys->clip_oversamples == 0, we jump into the final else clause, set *buf to temp, increment it, and are done.

    In other words, the fall-through is deliberate.



  • [quote user="fourchan"]

    I'm surprised noone has pointed out the obvious solution yet:

     

    if (asys->clip_oversamples)
      {
      if (temp>255) temp=255;else if (temp<0) temp=0;
      }
    *buf++ = temp; 

     

    [/quote]

     

    That's fine iff the unclipped value of temp isn't needed later in the routine.




  • Jumping into the middle of an "else" is not acceptable in most programming languages, but it works just fine in C.  Likewise with falling through to the next case in a case statement.  C is sometimes called a "glorified assembly language".  Like assembly language, the number of passes the compiler must make is very small, and each line generates a few instructions without much need to refer to context.

     If a is an array of int, the following two statements are equivalent:

        a[5] = 3;

        5[a] = 3;

    This is because the bracket notation is just syntactic sugar.  The compiler first converts the statements to

       *(a + 5) = 3;

       *(5 + a) = 3;

    Most languages would forbid this.  A few would be embarassed by it.  C is actually proud of it!

     



  • [quote user="mrprogguy"][quote user="welcor"][quote user="PSWorx"]

    ...and readable.

    [/quote]

    Readable ??!? particularly the missing break; makes it very unreadable indeed.

    How about this?

     if (asys->clip_oversamples == 1)
     {
       if (temp > 255)
          *buf++ = 255;
      else if (temp < 0)
          *buf++ = 0;
      else
          *buf++ = temp;
      }
      else if (asys->clip_oversamples == 0)
      {
        *buf++ = temp;
      }

    I know - it duplicates one line of code, but is otherwise at least ten times as readable.

    [/quote]

    Not too good, actually.  From context, asys->clip_oversamples is a boolean test, so the test for 0 is redundant.  Either the value is 1 or 0; if it ain't 1, then it's 0--no need for the extra test.  The combined switch/if construct is perfectly legal C/C++, and probably even generates decent machine code when compiled.  Those of you that aren't comfortable with letting cases fall through--and even seem to think that there's an implementation fault with this--just haven't been coding long enough, or believe too much of what your Java teachers are selling.

    Those of you who have only programmed at the C/C++ level and don't know assembly language don't realize that a decent compiler does switch() compilations as jump tables.  That's vastly more efficient than the ladder of cmp (type) instructions the compiler has to generate otherwise;  those that think the MIN/MAX macros are going to help the situation are wrong, because the compiler output of those will be a sorry mess.  The ?: stuff looks cute in code, but generally produces worse assembly than a regular if/then/else.  Seriously.

    As written, good.  As amended...well, sad.

    So say I, after 30 years of programming.

    [/quote]

    Well said, I second this opinion.  This is good code, and nobody here has yet to post an improvement on it.  The min/max solution will make the assembly a mess (notice that they introduce a redundant size check even if the first condition is met).  This is obviously optimized code and readability comes second.  Of course, it would have been nice to take the clip_oversamples test outside of the loop completely, but perhaps there was a reason the author could not do that.  For the "what if clip_oversamples is not 0 or 1" people, simply replace "case 1" with "default".



  • Now, there isn't any context around the code to know for sure, but if temp's original unclamped value isn't needed after this chunk of code, I'd do this:


         if (asys->clip_oversamples == 1)
        {
            if (temp > 255)
                temp = 255;
            else if (temp < 0)
                temp = 0;
        }

        *buf++ = temp;


    It might be a little slower due to the assigns if temp isn't a register, but I find it quite a bit more readable than the original.

    P.S.: I just spent 20 minutes trying to get my post to look right, and gave up in the end. Any pointers on how to get code to be monospace, etc? 



  • If you don't trust the compiler at all and you're already going to sacrifice readability, portability shouldn't be a big concern, so how about just using a few SSE/Altivec intrinsics (or inline assembly) and calling it a day. After all, this is exactly what they were put here to do.



  • Amateurs.

    *buf++=((asys->clip_oversamples)?((temp>255)?255:((temp<0)?0:temp)):temp);



  • @CDarkLock said:

    Amateurs.

    *buf++=((asys->clip_oversamples)?((temp>255)?255:((temp<0)?0:temp)):temp);

    								</div>
    								</div><p></blockquote><br><br>[insert "goggles" cliche here]</p><p>&nbsp;</p>


  • @CDarklock said:

    Amateurs.

    *buf++=((asys->clip_oversamples)?((temp>255)?255:((temp<0)?0:temp)):temp);

     

    Love it!  My kind of code.  



  • @CDarklock said:

    Amateurs.

    *buf++=((asys->clip_oversamples)?((temp>255)?255:((temp<0)?0:temp)):temp);

    Oh yeah. That surely is more readable. </sarcasm>

    Few things in C are more annoying than the misuse of ?: operator. It improves neither readability nor performance. For added evilness combine it with GCC's compound expressions:

    *buf++ = ((asys->clip_oversamples)? ({ temp = (temp>255?255:temp); temp = (temp<0?0:temp); temp; }) : temp);

    Even more evil (and more efficient) would be to rewrite this using conditional moves in assembly.



  • @Tweenk said:

    @CDarklock said:

    Amateurs.

    *buf++=((asys->clip_oversamples)?((temp>255)?255:((temp<0)?0:temp)):temp);

    Oh yeah. That surely is more readable. </sarcasm>

    *buf++= 
        asys->clip_oversamples
             ? ((temp>255)
                    ? 255
                    : (temp < 0 ? 0 : temp))
             : temp;
    


  • @fennec said:

    *buf++= 
    asys->clip_oversamples
    ? ((temp>255)
    ? 255
    : (temp < 0 ? 0 : temp))
    : temp;

    Now that is something different.
    Another option is to use GCC's C++ max / min operators (actually why did they deprecate them?). This is super-concise!

    if(asys->clip_oversamples) *buf++ = (temp >? 0) <? 255;
     


Log in to reply