[if - else if - else] oder [switch - case - default] ?

Eisflamme

klöklö schrieb:

Der Compiler wird daraus eine Sprungtabelle machen, denke ich. Was Lichtlein sagen wollte war das man z.B. nicht ausversehen if (x = a) schreiben kann.

Ach komm, ich bin der Meister der Flüchtigkeitsfehler, aber das ist mir die letzten 5 Jahre nicht passiert.

hustbaer

volkard schrieb:

ipsec schrieb:

Schön wären natürlich reale Beispiele

Dann wird's bei drei Fällen bleiben, vermute ich:
- Menüschleife in Konsolenanwendung,
- switch über enum,
- Duffs Device.

Weil ich das grad interessant finde, hab ich beschlossen mal schnell über den Code den ich in den letzten paar Monaten geschrieben habe zu greppen.

Insgesamt hab' ich da drinnen 65x switch gefunden, davon geht der überwiegende Grossteil auf enum Typen.

Zwei mal gibt's ein switch(charset), wobei charset ein int ist, aber hier auch "enum-artig" verwendet wird.

Dann gibt's 1x das hier (in einer Schleife die einen 3x4 Nummernblock bastelt):

for (int i = 0; i < 12; i++)
{
	// ...

	int const row = i / 3;
	int const column = i % 3;

	if (row == 3)
	{
		switch (column)
		{
		case 0:
			key->SetKeyType(KeyType_Clear);
			break;
		case 1:
			key->SetKeyType(KeyType_Digit);
			key->SetValue(0);
			break;
		case 2:
			key->SetKeyType(KeyType_Backspace);
			break;
		}
	}
	else
	{
		key->SetKeyType(KeyType_Digit);
		key->SetValue(i + 1);
	}

Hier könnte man noch behaupten dass das auch "enum artig" ist - ist aber schon grenzwertig.

Und in einer "BoxMinify" Funktion findet sich folgendes:

switch (xFactor)
	{
	case 1:
		for (int x = 0; x < destSize.x; x++)
			tmp[x] += s[x];
		break;

	case 2:
		for (int x = 0; x < destSize.x; x++)
			tmp[x] += s[2 * x] + s[2 * x + 1];
		break;

	case 3:
		for (int x = 0; x < destSize.x; x++)
			tmp[x] += s[3 * x] + s[3 * x + 1] + s[3 * x + 2];
		break;

	case 4:
		for (int x = 0; x < destSize.x; x++)
			tmp[x] += s[4 * x] + s[4 * x + 1] + s[4 * x + 2] + s[4 * x + 3];
		break;

	default:
		for (int x = 0; x < destSize.x; x++)
		{
			// accumulate pixels in "accu"
			UINT accu = 0;
			for (int j = 0; j < xFactor; j++)
				accu += s[j];

			tmp[x] += accu;
			s += xFactor;
		}
		break;
	}

Das ist für mich jetzt ganz klar nicht mehr "enum artig", da sämtliche integralen Skalierungsfaktoren > 0 unterstützt werden.

Also je nachdem was man noch gelten lässt 1 bis max. 4 aus 65.

BTW... andere Konstrukte befinden sich mit folgender Häufigkeit in dem Code:

30000 lines of code
1300 if (+ 120 else-if)
170 throw
71 for
68 catch
65 switch
24 while
3 do-while (davon 2 do{ ... }while(false) in Makros)

XSpille

hustbaer schrieb:

30000 lines of code
1300 if (+ 120 else-if)
170 throw
71 for
68 catch
65 switch
24 while
3 do-while (davon 2 do{ ... }while(false) in Makros)

Nur mal aus Interesse:
Wieviele verschiedene Exception-Typen schmeisst du denn?
Oder welches Exception-Klassen/Klassen-Verhältnis hast du?

volkard

hustbaer schrieb:

Dann gibt's 1x das hier (in einer Schleife die einen 3x4 Nummernblock bastelt):

for (int i = 0; i < 12; i++)
{
	// ...

	int const row = i / 3;
	int const column = i % 3;

	if (row == 3)
	{
		switch (column)
		{
		case 0:
			key->SetKeyType(KeyType_Clear);
			break;
		case 1:
			key->SetKeyType(KeyType_Digit);
			key->SetValue(0);
			break;
		case 2:
			key->SetKeyType(KeyType_Backspace);
			break;
		}
	}
	else
	{
		key->SetKeyType(KeyType_Digit);
		key->SetValue(i + 1);
	}

Hmm. Das Keypad ist 4 hoch und 3 breit. Nur wenn die row 3 ist, also nur die letze, macht: Nacheinander case0, case1, case2. Dann könnte ich das Nacheinander auch Am Ende nachschicken und den Code verkürzen zu:

for(int row=0; row!=3; ++row) {
		for(int column=0; column!=3; ++column) {
			// ...
			key->SetKeyType(KeyType_Digit);
			key->SetValue(row*3+column + 1);
		}
	}
	key->SetKeyType(KeyType_Clear);
	key->SetKeyType(KeyType_Digit);
	key->SetValue(0);
	key->SetKeyType(KeyType_Backspace);

volkard

hustbaer schrieb:

Und in einer "BoxMinify" Funktion findet sich folgendes:

switch (xFactor)
	{
	case 1:
		for (int x = 0; x < destSize.x; x++)
			tmp[x] += s[x];
		break;

	case 2:
		for (int x = 0; x < destSize.x; x++)
			tmp[x] += s[2 * x] + s[2 * x + 1];
		break;

	case 3:
		for (int x = 0; x < destSize.x; x++)
			tmp[x] += s[3 * x] + s[3 * x + 1] + s[3 * x + 2];
		break;

	case 4:
		for (int x = 0; x < destSize.x; x++)
			tmp[x] += s[4 * x] + s[4 * x + 1] + s[4 * x + 2] + s[4 * x + 3];
		break;

	default:
		for (int x = 0; x < destSize.x; x++)
		{
			// accumulate pixels in "accu"
			UINT accu = 0;
			for (int j = 0; j < xFactor; j++)
				accu += s[j];

			tmp[x] += accu;
			s += xFactor;
		}
		break;
	}

Ja, den Fall habe ich klar vergessen. Eine Variable compilezeitkonstant machen, damit sie als Template-Argument geht oder dem Optimierer zugänglich wird.

Ich hab da mal ein wenig experimentiert...

#include <iostream>

struct Size {
    int x;
};

typedef unsigned int UINT;

void helper(UINT* tmp,UINT* s,int xFactor,Size& destSize){
    for (int x = 0; x < destSize.x; x++) {
        // accumulate pixels in "accu"
        UINT accu = 0;
        for (int j = 0; j < xFactor; j++)
            accu += s[j];

        tmp[x] += accu;
        s += xFactor;
    }
}

void boxMinifyTest(UINT* tmp,UINT* s,int xFactor,Size& destSize) {
	switch (xFactor) {
	case 1:
        helper(tmp,s,1,destSize);
		break;

	case 2:
        helper(tmp,s,2,destSize);
		break;

	case 3:
        helper(tmp,s,3,destSize);
		break;

	case 4:
        helper(tmp,s,4,destSize);
		break;

	default:
        helper(tmp,s,xFactor,destSize);
		break;
	}
}

int main() {
    UINT tmp[100];
    UINT s[100];
    Size size;
    std::cin>>size.x;
    int xFactor;
    std::cin>>xFactor;
    boxMinifyTest(tmp,s,xFactor,size);
	return 0;
}

Und der entstehende Code ist faszinierend.

.LFE1237:
	.size	_Z6helperPjS_iR4Size, .-_Z6helperPjS_iR4Size
	.p2align 4,,15
.globl _Z13boxMinifyTestPjS_iR4Size
	.type	_Z13boxMinifyTestPjS_iR4Size, @function
_Z13boxMinifyTestPjS_iR4Size:
.LFB1238:
	.cfi_startproc
	pushq	%rbp
	.cfi_def_cfa_offset 16
	cmpl	$2, %edx        #switch gebaut, vergleich mit 2
	pushq	%rbx
	.cfi_def_cfa_offset 24
	je	.L17            #wenn gleich 2, dann dahin
	.cfi_offset 3, -24
	.cfi_offset 6, -16
	jle	.L49            #wenn kleiner 2 dann dahin
	cmpl	$3, %edx
	je	.L18            #wenn gleich 3 dann dahin
	cmpl	$4, %edx
	.p2align 4,,5
	jne	.L15            #wenn ungleich gleich 4 dann dahin
	movl	(%rcx), %r10d   
	xorb	%dl, %dl
	testl	%r10d, %r10d
	jle	.L14            #wenn size==0, return
	.p2align 4,,7
	.p2align 3
.L40:                           #kopierschleife mit viererschleife geunrolled
	movl	(%rsi), %eax
	addl	(%rdi), %eax
	incl	%edx
	addl	4(%rsi), %eax
	addl	8(%rsi), %eax
	addl	12(%rsi), %eax
	addq	$16, %rsi
	movl	%eax, (%rdi)
	addq	$4, %rdi
	cmpl	(%rcx), %edx
	jl	.L40
	.p2align 4,,7
	.p2align 3
.L14:                          #return
	popq	%rbx
	.cfi_remember_state
	.cfi_def_cfa_offset 16
	popq	%rbp
	.cfi_def_cfa_offset 8
	ret
	.p2align 4,,7
	.p2align 3
.L49:
	.cfi_restore_state
	cmpl	$1, %edx           #noch switch: wenn 1 dann
	je	.L50               #dahin
.L15:
	movl	(%rcx), %ebx       #ah, es war die 0 oder ungleich 4
	testl	%ebx, %ebx         #es folgt schleife ohne unrolling mit viel mmx
	.p2align 4,,3
	jle	.L14
	movl	%edx, %r10d
	movslq	%edx, %rbp
	xorl	%r11d, %r11d
	shrl	$2, %r10d
	salq	$2, %rbp
	leal	0(,%r10,4), %ebx
	.p2align 4,,7
	.p2align 3
.L26:
	xorl	%r9d, %r9d
	testl	%edx, %edx
	jle	.L34
	cmpl	$7, %edx
	jbe	.L37
	testl	%ebx, %ebx
	je	.L37
	pxor	%xmm0, %xmm0
	movq	%rsi, %r8
	xorl	%eax, %eax
	.p2align 4,,7
	.p2align 3
.L31:
	movdqu	(%r8), %xmm1
	incl	%eax
	addq	$16, %r8
	cmpl	%r10d, %eax
	paddd	%xmm1, %xmm0
	jb	.L31
	movdqa	%xmm0, %xmm1
	cmpl	%edx, %ebx
	movl	%ebx, %eax
	psrldq	$8, %xmm1
	paddd	%xmm1, %xmm0
	movdqa	%xmm0, %xmm1
	psrldq	$4, %xmm1
	paddd	%xmm1, %xmm0
	movd	%xmm0, -4(%rsp)
	movl	-4(%rsp), %r9d
	je	.L34
.L35:
	movslq	%eax, %r8
	leaq	(%rsi,%r8,4), %r8
	.p2align 4,,7
	.p2align 3
.L33:
	incl	%eax
	addl	(%r8), %r9d
	addq	$4, %r8
	cmpl	%eax, %edx
	jg	.L33
.L34:
	addl	%r9d, (%rdi)
	incl	%r11d
	addq	$4, %rdi
	cmpl	(%rcx), %r11d
	jge	.L14
	addq	%rbp, %rsi
	jmp	.L26
.L50:                           #Fall 1
	movl	(%rcx), %ebp
	xorl	%eax, %eax
	xorl	%edx, %edx
	testl	%ebp, %ebp
	jle	.L14            #wenn size==0, dann return
	.p2align 4,,7
	.p2align 3
.L41:
	movl	(%rsi,%rax), %ebx  #kopierschleife, innere schleife geunrolled, 
	addl	%ebx, (%rdi,%rax)  #äußeren schleifenkörper länger gemacht, dafür zwei ausstiege
	incl	%edx
	addq	$4, %rax
	cmpl	(%rcx), %edx
	jge	.L14
	movl	(%rsi,%rax), %ebx
	addl	%ebx, (%rdi,%rax)
	incl	%edx
	addq	$4, %rax
	cmpl	(%rcx), %edx
	jl	.L41
	jmp	.L14
.L18:                          # gleich 3
	movl	(%rcx), %r9d   # wenn size==0, dann return
	xorl	%edx, %edx
	testl	%r9d, %r9d
	jle	.L14
	.p2align 4,,7
	.p2align 3
.L39:                          #kopierschliefe, die innere dreierschleife geunrolled
	movl	(%rsi), %eax
	addl	(%rdi), %eax
	incl	%edx
	addl	4(%rsi), %eax
	addl	8(%rsi), %eax
	addq	$12, %rsi
	movl	%eax, (%rdi)
	addq	$4, %rdi
	cmpl	(%rcx), %edx
	jl	.L39
	jmp	.L14          #return
	.p2align 4,,7
	.p2align 3
.L17:                            #Fall 2
	movl	(%rcx), %r8d     # wenn size==0, dann return
	xorl	%edx, %edx
	testl	%r8d, %r8d
	jle	.L14
	.p2align 4,,7
	.p2align 3
.L38:                            #kopierschliefe, die innere zweierschleife geunrolled
	movl	(%rsi), %eax
	addl	(%rdi), %eax
	incl	%edx
	addl	4(%rsi), %eax
	addq	$8, %rsi
	movl	%eax, (%rdi)
	addq	$4, %rdi
	cmpl	(%rcx), %edx
	jl	.L38
	jmp	.L14            #return
.L37:
	xorl	%r9d, %r9d
	xorl	%eax, %eax
	jmp	.L35
	.cfi_endproc

Mal sehen, wieviel sich der Compiler bieten läßt...

switch (xFactor) {
	case  1: helper(tmp,s, 1,destSize); break;
	case  2: helper(tmp,s, 2,destSize); break;
	case  3: helper(tmp,s, 3,destSize); break;
	case  4: helper(tmp,s, 4,destSize); break;
	case  5: helper(tmp,s, 5,destSize); break;
	case  6: helper(tmp,s, 6,destSize); break;
	case  7: helper(tmp,s, 7,destSize); break;
	case  8: helper(tmp,s, 8,destSize); break;
	case  9: helper(tmp,s, 9,destSize); break;
	case 10: helper(tmp,s,10,destSize); break;
	case 11: helper(tmp,s,11,destSize); break;
	case 12: helper(tmp,s,12,destSize); break;
	case 13: helper(tmp,s,13,destSize); break;
	case 14: helper(tmp,s,14,destSize); break;
	case 15: helper(tmp,s,15,destSize); break;
	case 16: helper(tmp,s,16,destSize); break;
	case 17: helper(tmp,s,17,destSize); break;
	case 18: helper(tmp,s,18,destSize); break;
	case 19: helper(tmp,s,19,destSize); break;
	case 20: helper(tmp,s,20,destSize); break;

	default: helper(tmp,s,xFactor,destSize); break;
	}

Jetzt ist das switch zu einer Sprungtabelle geworden und alle 20 sind innen geunrolled.

Ich habe noch mal überlegt weil mir if/else so gar nicht gefallen wollte.
Und es gibt Unterschiede.

Ratespiel: Verhält sich der Code immer gleich?

switch(*ptr)
{
case 0x11: mach_was_zu11(); break;
case 0x64: mach_was_zu64(); break;
default:   mach_default_was(); break;
}

if(*ptr == 0x11) mach_was_zu11(); 
else if(*ptr == 0x64) mach_was_zu64;
else mach_default_was();

Nein, wenn die variable 'ptr' auf ein volatile Type zeigt.
Dann ist das Verhalten ganz Unterschiedlich.

Beispiel währe hier in einer Multithread Anwendung oder wenn man Hardware Register ausliest.

Lichtlein

MaPoX

Lichtlein schrieb:

Dann ist das Verhalten ganz Unterschiedlich.

Dann erläuter doch mal den Unterschied
Ich sehe nur, dass er im if strenggenommen 2mal den Wert in das Register
laden muss. Ehrlich gesagt glaube ich aber nicht, dass er es macht...
Der Unterschied wäre ja eh nicht kontrollierbar (Thread-Synchronisierung) für den Benutzer innerhalb des Konstruktes.

Nexus

MaPoX schrieb:

Ich sehe nur, dass er im if strenggenommen 2mal den Wert in das Register
laden muss. Ehrlich gesagt glaube ich aber nicht, dass er es macht...

Mit volatile hat er es zu machen.

hustbaer

@Lichtlein:
Super-sinnloses Beispiel.
Da hier keinerlei Synchronisierungs-Punkte zu sehen sind, verhält sich der Code in beiden Fällen genau *irgendwie*.

Einzig wenn das auf was "ptr" zeigt ein sog. "Strobe-Register" ist könnte hier was sinnvolles rauskommen. Nur dass keine Sau mehr "Strobe-Register" verwendet. (Seit dem Amiga sind mir keine mehr untergekommen)
(Es müsste auch ein Lese-aktiviertes "Strobe-Register" sein, und die sind gleich noch viel seltener)

XSpille

@Lichtlein: Blödes Beispiel

Dein in der Regel nicht spürbarer Unterschied ist nur an den Datentyp geknüpft.
Dann kannst du auch gleich ein Beispiel produzieren, das ein spürbar
unterschiedliches Verhalten hat:

#include <iostream>

class Foo{
public:
        Foo() : i(2){}

        operator int(){
                return i--;
        }
private:
        int i;
};

void if_test(Foo& foo){
        if(foo == 0){
                std::cout << "0" << std::endl;
        } else if(foo == 1){
                std::cout << "1" << std::endl;
        } else {
                std::cout << "2" << std::endl;
        }
}

void switch_test(Foo& foo){
        switch(foo){
        case 0:
                std::cout << "0" << std::endl;
                break;
        case 1:
                std::cout << "1" << std::endl;
                break;
        default:
                std::cout << "2" << std::endl;

        }
}

int main(){
        Foo foo;
        if_test(foo);
//      switch_test(foo);       
}

if_test:

1

switch_test:

2

Gruß,
XSpille

EDIT: Ich weiß, dass dieses Beispiel auch sinnlos ist

Hier mal ein Beispiel aus der Echtzeitprogrammierung.

Gegeben ist ein Hardware Status Register. Aus dem Register bekommt man eine
Zahl zurück die folgend Kodiert ist.

0 = Kein Ereignis
1 = Empfangs Buffer Belegt ( hat hoehere prio )
2 = Sende Buffer Frei
3 = Fehler
x = Hardware Error

Ändert sich am Status was so wird in Programm eine Funktion aufgerufen die macht folgendes. (Ich bilde jetzt nur mal die if/else um das Fehlverhalten zu zeigen. Mit switch könnt Ihr es euch selber ausdenken :-))

volatile struct sDevice
{
  uint32_t   status;
  uint32_t   command;
};

/* 
Funktion wird aufgerufen weil der sende buffer frei wird.
status hat den inhalt 2.
*/

bool funktion( sDevice *pDev)
{
   if(pDev->status == 0)
     return false;
   else 
   if(pDev->status == 1)
     return writeBuffer(pDev);
   else

/*
hier wird das Programm vom Betriebssystem unterbrochen.
bevor man wieder rechenzeit bekommt. In der Zeit meldet das
Device eine neue Message im Buffer.
im status steht jetzt 1.

Und was passiert jetzt im Program, na ?
*/

   if(pDev->status == 2)
     return readBuffer(pDev);
   else
   if(pDev->status == 3)
     return resetDevice(pDev);
   else
   {
     panic();
     stopSystem();
   }
}

Hoffe das ist ersichtlich das hier ein Fehler passiert.

Lichtlein

Ist das nicht einfach bloss schlecht kodiert? Der Status sollte kopiert werden, nicht ein Pointer übergeben... wäre ja nicht mal Thread safe.

XSpille

Es ist logisch, dass zusätzliche Operationen das Ergebnis des Programms
verändern können. Ob es jetzt ein Funktionsaufruf ist, der etwas zurückliefert
oder eine Konvertierungsfunktion.

Dort findet ein zusätzliches Laden eines Wertes statt.
Da kannst du genauso einen Funktionsaufruf reinpacken, ist für mich
inhaltlich das Gleiche.

@schlecht

Ist bei Device Treiber gängige Paxis und Thread safe braucht es auch nicht sein. Der Treiber an sich ist immer Single Thread (In der ISR) . Kann aber durch das Betriebssystem unterbrochen werden.

@XSpille

Darum ging es mir ja, das sich was ändern kann, das eben ein if/else nicht gleichzusetzen ist mit einen switch.

Noch mal zu den Beispiel.
Es ist ja nicht so das ich mir das aus den Finger gesogen habe. Häufig entsteht solcher Code über lange Zeit weil der Chip sich weiter Entwickelt und neue Funktionen bekommt. Und dann lass jemanden am Treiber Programmieren der aus der Anwendungs-Entwicklung kommt. Da kommen Sachen raus.

Also, benutzt switch/case wann immer möglich. Ist leichter zu warten.

Lichtlein

volkard

Lichtlein schrieb:

Also, benutzt switch/case wann immer möglich. Ist leichter zu warten.

April, April!

hustbaer

@Lichtlein:
Vorweg: ja, du hast Recht, dein Beispiel "funktioniert" (in dem Sinn dass sich da mit if-else-if ein Problem ergibt welches sich mit switch() nicht ergibt. I stand corrected.

----

Ich hab schon einigen Treiber-Code gesehen, aber noch keinen derart schrottigen.

Es ist ja nicht so das ich mir das aus den Finger gesogen habe. Häufig entsteht solcher Code über lange Zeit weil der Chip sich weiter Entwickelt und neue Funktionen bekommt. Und dann lass jemanden am Treiber Programmieren der aus der Anwendungs-Entwicklung kommt. Da kommen Sachen raus.

Man kann die Zugriffe auch in ReadXxxRegister() und WriteYyyRegister() Funktionen packen. Dann versteht auch Mr. Anwendungsprogrammiererwastler was da abgeht. Mal ganz davon abgesehen dass Mr. Anwendungsprogrammiererwastler in der Treiberentwicklung nix verloren hat. Zumindest nicht ohne Einschulung.

Was als Aussage für mich übrig bleibt: schlechte Programmierer schreiben schlechte Programme. Das lässt sich aber mMn. nicht dadurch fixen/verbessern dass man ihnen verbietet if-else-if zu verwenden (bzw. empfiehlt switch stattdessen zu verwenden). Auch wenn manche anderen Programmierer glauben dass es ne super Taktik ist um Probleme zu vermeiden.

abc.w

Lichtlein schrieb:

Also, benutzt switch/case wann immer möglich. Ist leichter zu warten.

Habe heute in einem Treiber diesen Code gesehen und an diesen Thread hier gedacht:

switch (value)
{
    default:
        ...
        blabla(value);
        ...
        break;
}

... ja, ein switch ohne case ist sogar noch einfacher zu warten

Ah, ein Treiberentwickler der Zukünftige Entwicklungen berücksichtigt !! Sehr Löblich.

Lichtlein

abc.w

Genau, toller Code!
Es gibt da noch mehr Sachen, z.B. switch mit nichts:

switch (value)
{
    default:
        ;
}

oder ein switch-case ohne default-Zweig:

switch (value)
{
    case A:
        ...
        break;
    case B:
        ...
        break;
}

oder ein switch-case mit einem mekrwürdigen break:

switch (value)
{
    case A:
        ...
        ret = blabla();
        ...
        if (ret)
            break;
    case B:
    case C:
        break;
}

in case B und C steht kein Code, einfach break. Toll...

Nexus

abc.w schrieb:

oder ein switch-case ohne default-Zweig:

Wenn bereits vorhin alle möglichen Fälle (in denen etwas passiert) abgedeckt sind, ist default unnötig. Du schreibst ja auch nicht zu jedem if ein else .

abc.w schrieb:

in case B und C steht kein Code, einfach break. Toll...

Wo liegt das Problem? So macht man wenigstens explizit, dass es noch die Fälle B und C gibt und dass in diesen Fällen nichts getan wird. Noch ausdrücklicher ginge es nur noch mit einem Kommentar.