Unnötige Compilerwarnungen

cooky451

/W4 hat zwar wirklich einige Stellen an denen es über's Ziel hinaus schießt, aber hier finde ich, sind alle berechtigt.
1. + 2. Ich bin der Meinung, dass jedes switch ein default definieren haben haben sollte.
3. Ja, machst du denn noch was mit inserted? Du holst das doch nicht nur um zu assert()en?
4. Den operator als = delete zu definieren ist expliziter, ich denke das ist keine so schlechte Sache. Zumal ich Referenzen als Membervariablen eh fragwürdig finde.
5. Ja, so etwas mag hier richtig sein, ist aber grundsätzlich fehleranfälliger, als die Zuweisung einfach im Körper zu machen. Falls man das wirklich unbedingt als Initialisierung braucht, müsste man sich was überlegen.

Was ich mache ist wohl "den Code unnötig verkomplizieren". Wobei ich normalerweise auch mit /W3 kompiliere, und dann nur ab und an mal mit /W4, wenn dann 1-2 Warnungen sitzen bleiben die einfach nicht zu beheben sind, dann passt das schon. Die Mühe einzelne Warnungen auszuschalten oder Ähnliches mache ich mir nie, das artet nur aus. Einzige Ausnahme: -D _CRT_SECURE_NO_WARNINGS, das benutze ich immer mit VS.

Marthog schrieb:

1. Der Code ist zwar richtig, aber wenn in einem switch ein enum abgearbeitet wird, ist es möglich, dass der Programmierer allen Werten eine bestimmte Bedeutung zugewiesen hat und nicht nur ein paar.
Der Compiler ist hier nicht überempfindlich, sondern weist auch vergessene Anweisungen hin, es ist ja möglich, dass du C einfach nur vergessen hast.
Man kann die Warnung einfach mit
default: break;
verhindern, das macht eine Zeile mehr.

Ich würde da je nach Anwendungsfall eher sowas hinschreiben:

int foo(Enum e)
{
  switch(e)
  case A:
    return 0;
  case B:
    return 1;
  default:
    assert(false);
}

(Gerade auch in Verwendung mit der zweiten Frage.)

Nexus

cooky451 schrieb:

1. + 2. Ich bin der Meinung, dass jedes switch ein default definieren haben haben sollte.

Okay.

cooky451 schrieb:

3. Ja, machst du denn noch was mit inserted? Du holst das doch nicht nur um zu assert()en?

Doch, warum nicht?

cooky451 schrieb:

4. Den operator als = delete zu definieren ist expliziter, ich denke das ist keine so schlechte Sache. Zumal ich Referenzen als Membervariablen eh fragwürdig finde.

Versuch das mal mit VS 2010

Das Problem betrifft ja nicht nur Referenzen, sondern nicht-zuweisbare Objekte im Allgemeinen. Und Referenzen finde ich für Objekte ohne Wertsemantik okay, auch wenn ich (nicht zuletzt wegen dieser Warnung) sie inzwischen sehr selten habe.

Sky@logoff schrieb:

Ich würde da je nach Anwendungsfall eher sowas hinschreiben:
default:
    assert(false);

Ja, aber dafür scheinen Compiler nicht klug genug zu sein. Man muss noch ein return oder throw hinterherhängen. Ich habe mir ernsthaft schon überlegt, ob ich mir ein Makro für nicht-gültiges default: schreiben soll, sodass ich deswegen sicher keine Logikfehler einbaue.

cooky451

Nexus schrieb:

Doch, warum nicht?

Weil assert() für mich dafür da ist um harte Programmierfehler/Logikfehler im eigenen Code (keine Parameter) aufzudecken. Insofern würde ich es nie in einem Kontext nutzen in dem assert() das "primäre Ziel" ist, das macht für mich keinen Sinn, wenn ich einen Container überprüfen will oder so, würde ich eher eine Exception werfen.

camper

Sky@logoff schrieb:

Ich würde da je nach Anwendungsfall eher sowas hinschreiben:
int foo(Enum e)
{
  switch(e)
  case A:
    return 0;
  case B:
    return 1;
  default:
    assert(false);
}
(Gerade auch in Verwendung mit der zweiten Frage.)

Da fehlt dann allerdings wieder ein return oder abort.
Edit: hat Nexus schon gesagt...

Nexus

cooky451 schrieb:

Weil assert() für mich dafür da ist um harte Programmierfehler/Logikfehler im eigenen Code (keine Parameter) aufzudecken. Insofern würde ich es nie in einem Kontext nutzen in dem assert() das "primäre Ziel" ist, das macht für mich keinen Sinn, wenn ich einen Container überprüfen will oder so, würde ich eher eine Exception werfen.

Ursprünglich geht es um Ressourcen, die geladen und in einer Map abgespeichert werden. Da es in dem Fall ein Logikfehler ist, eine Ressource mehrfach zu laden, habe ich mich für assert entschieden. Ist also interner Code, nicht irgendein API. Obwohl ich auch durchaus durch Parameter gegebene Preconditions in Funktionen mit assert prüfe.

Um noch einmal auf das konkrete Beispiel 2 zurückzukommen: Hier gings darum, Enumeratoren von einem Enum auf das andere abzubilden.

Enum2 convert(Enum1 value) 
{ 
     switch (value) 
     { 
         case A: return X; 
         case B: return Y; 
         case C: return Z; 
     }
}

Wie sähe euer Code hier aus? Bei einem default: müsste man ebenfalls etwas tun, ich möchte in diesem (niemals auftretenden) Fall nicht einfach einen willkürlich gewählten Enumerator zurückgeben.

Vielleicht throw std::logic_error(...) und vorher assert(false) ? Was mir dann wieder nicht gefällt, ist dass das nie auftretende throw je nach Implementierung trotzdem teuer werden kann. Überhaupt finde ich das alles äusserst fragwürdig, wenn man bedenkt, dass ich den ganzen Aufwand nur zur Behebung einer Warnung in Kauf nehme.

Edit: Unter MSVC scheint __assume genau dafür da zu sein. Läuft wohl immer mehr auf ein Makro hinaus...

cooky451

Wenn's ein Logikfehler ist der nie auftreten kann würde ich erst asserten und dann std::terminate aufrufen.

Enum2 convert(Enum1 value) 
{ 
     switch (value) 
     {
         default: assert(false && "Don't!"); std::terminate();
         case A: return X; 
         case B: return Y; 
         case C: return Z; 
     } 
}

Wenn's nur ein falscher Parameter ist, würde ich vermutlich throwen, je nach Kontext. Solange das nicht auf einer Armbanduhr läuft.

TyRoXx

///undefined behaviour if called with a value not from the enum
Enum2 convert(Enum1 value) 
{
     switch (value) 
     {
         case A: return X;
         case B: return Y;
         case C: return Z;
		//kein default, damit der Compiler bei vergessenen cases warnt
     }
     assert(false && "Invalid value in argument 'value'");
     return Enum2::SomeValue;
}

Nutze undefiniertes Verhalten, um keine premature pessimization zu haben. enum class schadet auch nie. Noch besser: Nutze keine Enums mit switch .

EDIT: default aus Code-Beispiel entfernt, um eine berechtigte Warnung nicht zu unterdrücken.
EDIT2: Für genau so etwas sollte C++ eigentlich Mittel haben, um eine unerreichbare Stelle zu kennzeichnen. Sozusagen explizites undefiniertes Verhalten, damit man auch noch den generierten Code für return Enum2::SomeValue sparen kann. Das Weglassen des return führt bloß zu einer Warnung. Und nein: Weder abort , noch terminate noch throw haben den gewünschten Effekt, denn sie generieren Code und haben unnötigerweise eine definierte Wirkung.
AFAIK hat das Erzeugen von enum class -Variablen aus unpassenden Werten leider ein definiertes Verhalten. Wenn das nicht so wäre, könnte man sich jegliche Behandlung anderer Werte sparen.

Skym0sh0

Sondern?

Eisflamme