Dlaczego Clang / LLVM ostrzega mnie przed użyciem wartości domyślnej w instrukcji switch, w której uwzględniono wszystkie wyliczone przypadki?

34

Rozważ następującą instrukcję wyliczania i przełączania:

typedef enum {
    MaskValueUno,
    MaskValueDos
} testingMask;

void myFunction(testingMask theMask) {
    switch (theMask) {
        case MaskValueUno: {}// deal with it
        case MaskValueDos: {}// deal with it
        default: {} //deal with an unexpected or uninitialized value
    }
};

Jestem programistą Objective-C, ale napisałem to w czystym C dla szerszej publiczności.

Clang / LLVM 4.1 z -Wszystko ostrzega mnie w domyślnej linii:

Domyślna etykieta w przełączniku, która obejmuje wszystkie wartości wyliczeń

Teraz rozumiem, dlaczego tak jest: w idealnym świecie jedynymi wartościami wprowadzanymi do argumentu theMaskbyłyby wyliczenie, więc żadne domyślne nie jest konieczne. Ale co jeśli jakiś hack pojawi się i wrzuci niezainicjowaną int do mojej pięknej funkcji? Moja funkcja zostanie udostępniona jako zrzut w bibliotece i nie mam kontroli nad tym, co może się tam znaleźć. Używanie defaultjest bardzo zgrabnym sposobem radzenia sobie z tym.

Dlaczego bogowie LLVM uważają to zachowanie za niegodne ich piekielnego urządzenia? Czy powinienem poprzedzać to instrukcją if, aby sprawdzić argument?

Swizzlr
źródło
1
Powinienem powiedzieć, że moim powodem jest to, że jestem lepszym programistą. Jak NSHipster mówi : "Pro tip: Try setting the -Weverything flag and checking the “Treat Warnings as Errors” box your build settings. This turns on Hard Mode in Xcode.".
Swizzlr
2
-Weverythingmoże być przydatny, ale należy uważać na zbyt częste mutowanie kodu, aby sobie z tym poradzić. Niektóre z tych ostrzeżeń są nie tylko bezwartościowe, ale przynoszą efekt przeciwny do zamierzonego i najlepiej je wyłączyć. (Rzeczywiście, taki jest przypadek użycia -Weverything: zacznij od włączenia i wyłącz to, co nie ma sensu.)
Steven Fisher,
Czy mógłbyś rozwinąć nieco komunikat ostrzegawczy dla potomności? Zwykle jest w nich coś więcej.
Steven Fisher,
Po przeczytaniu odpowiedzi nadal wolę twoje początkowe rozwiązanie. Zastosowanie domyślnego stwierdzenia IMHO jest lepsze niż alternatywy podane w odpowiedziach. Tylko mała uwaga: odpowiedzi są naprawdę dobre i zawierają wiele informacji, pokazują fajny sposób rozwiązania problemu.
bbaja42,
@StevenFisher To było całe ostrzeżenie. I jak zauważył Killian, jeśli zmodyfikuję mój wyliczenie później, otworzy to możliwość przekazania prawidłowych wartości do domyślnej implementacji. Wydaje się, że to dobry wzorzec projektowy (jeśli jest to coś takiego).
Swizzlr

Odpowiedzi:

31

Oto wersja, która nie cierpi ani z powodu zgłaszania problemów, ani tej, przed którą się strzeżesz:

void myFunction(testingMask theMask) {
    assert(theMask == MaskValueUno || theMask == MaskValueDos);
    switch (theMask) {
        case MaskValueUno: {}// deal with it
        case MaskValueDos: {}// deal with it
    }
}

Killian wyjaśnił już, dlaczego brzęk emituje ostrzeżenie: jeśli rozszerzysz wyliczenie, wpadniesz w domyślny przypadek, który prawdopodobnie nie jest tym, czego chcesz. Właściwe jest usunięcie domyślnej skrzynki i otrzymanie ostrzeżeń o nieobsługiwanych warunkach.

Teraz obawiasz się, że ktoś może wywołać twoją funkcję z wartością spoza wyliczenia. To brzmi jak niespełnienie warunku wstępnego funkcji: udokumentowano, że oczekuje wartości z testingMaskwyliczenia, ale programista przeszedł coś innego. Zrób więc błąd programisty przy użyciu assert()(lub NSCAssert()jak powiedziałeś, że używasz Objective-C). Spraw, aby Twój program się zawiesił, wyświetlając komunikat wyjaśniający, że programista robi to źle, jeśli programista robi to źle.


źródło
6
+1. Jako małą modyfikację wolę mieć każdą sprawę return;i dodać znak assert(false);na końcu (zamiast powtarzać się, wymieniając prawne wyliczenia na początku assert()i na końcu switch).
Josh Kelley,
1
Co się stanie, jeśli niepoprawne wyliczenie nie zostanie przekazane przez głupiego programistę, ale przez błąd uszkodzenia pamięci? Kiedy kierowca naciśnie przerwę swojej Toyoty, a błąd zniszczył wyliczenie, program do obsługi przerw powinien się zawiesić i spalić, a kierowcy powinien zostać wyświetlony tekst: „programista robi to źle, zły programista! „. Nie do końca rozumiem, jak to pomoże użytkownikowi, zanim przejedzie przez krawędź urwiska.
2
@Lundin jest tym, co robi OP, czy jest to bezcelowy teoretyczny przypadek, który właśnie zbudowałeś? Niezależnie od tego, „błąd uszkodzenia pamięci” [i] jest błędem programisty, [ii] nie jest czymś, z czego można kontynuować w znaczący sposób (przynajmniej nie z wymaganiami określonymi w pytaniu).
1
@GrahamLee Mówię tylko, że „połóż się i umrzyj” niekoniecznie jest najlepszą opcją, gdy zauważysz nieoczekiwany błąd.
1
Ma nowy problem polegający na tym, że można pozwolić, aby asercja i sprawy przypadkowo zsynchronizowały się.
user253751
9

Posiadanie defaulttutaj etykiety jest wskaźnikiem, że nie rozumiesz, czego się spodziewasz. Ponieważ wszystkie enumwartości zostały jawnie wyczerpane , defaultprawdopodobnie nie można ich wykonać i nie trzeba ich też chronić przed przyszłymi zmianami, ponieważ jeśli rozszerzysz enum, to konstrukcja wygeneruje już ostrzeżenie.

Tak więc kompilator zauważa, że ​​obejrzałeś wszystkie podstawy, ale wydaje się, że myślisz , że nie, i to zawsze jest zły znak. Podejmując minimalne wysiłki, aby zmienić switchoczekiwaną formę, demonstrujesz kompilatorowi, że to, co wydaje się robić, jest tym, co faktycznie robisz, i wiesz o tym.

Kilian Foth
źródło
1
Ale moją troską jest brudna zmienna i ochrona przed tym (jak, jak powiedziałem, niezainicjowana int). Wydaje mi się, że w takiej sytuacji instrukcja switch mogłaby osiągnąć wartość domyślną.
Swizzlr
Nie znam definicji Objective-C na pamięć, ale założyłem, że wyliczenie typdef'd będzie wymuszone przez kompilator. Innymi słowy, „niezainicjowana” wartość może wejść do metody tylko wtedy, gdy twój program wykazuje już niezdefiniowane zachowanie, i uważam za całkowicie uzasadnione, że kompilator nie bierze tego pod uwagę.
Kilian Foth,
3
@KilianFoth nie, nie są. Wyliczenia celu C są wyliczeniami C, a nie wyliczeniami Java. Dowolna wartość z podstawowego typu całkowego może być obecna w argumencie funkcji.
2
Można również ustawić wyliczenia w czasie wykonywania, od liczb całkowitych. Mogą więc zawierać dowolną możliwą do wyobrażenia wartość.
OP jest poprawny. testingMask m; mojaFunkcja (m); najprawdopodobniej trafiłby w przypadek domyślny.
Matthew James Briggs,
4

Clang jest zdezorientowany, mając domyślne stwierdzenie, że istnieje doskonała praktyka, jest znany jako programowanie defensywne i jest uważany za dobrą praktykę programowania (1). Jest często używany w systemach o znaczeniu krytycznym, choć może nie w programowaniu na pulpicie.

Programowanie obronne ma na celu wychwycenie nieoczekiwanych błędów, które teoretycznie nigdy by się nie zdarzyły. Taki nieoczekiwany błąd niekoniecznie oznacza, że ​​programista podaje funkcji niepoprawne dane wejściowe, a nawet „zły hack”. Bardziej prawdopodobne jest, że może to być spowodowane uszkodzoną zmienną: przyczyną mogą być przepełnienia bufora, przepełnienie stosu, niekontrolowany kod i podobne błędy niezwiązane z funkcją. W przypadku systemów wbudowanych zmienne mogą się zmieniać z powodu zakłóceń elektromagnetycznych, szczególnie jeśli używasz zewnętrznych obwodów pamięci RAM.

Co do tego, co napiszę w domyślnej instrukcji ... jeśli podejrzewasz, że program zwariował, gdy tam trafiłeś, potrzebujesz jakiejś obsługi błędów. W wielu przypadkach prawdopodobnie możesz po prostu dodać puste zdanie z komentarzem: „nieoczekiwany, ale nie ma znaczenia” itp., Aby pokazać, że zastanowiłeś się nad nieoczekiwaną sytuacją.


(1) MISRA-C: 2004 15.3.


źródło
1
Nie należy zapominać, że przeciętny programista na PC zwykle uważa, że ​​koncepcja programowania obronnego jest całkowicie obca, ponieważ ich pogląd na program jest abstrakcyjną utopią, w której nic, co teoretycznie nie może się mylić, nie popełni się w praktyce. W związku z tym otrzymasz dość różnorodne odpowiedzi na swoje pytanie w zależności od tego, kogo zapytasz.
3
Clang nie jest zdezorientowany; wyraźnie poproszono o dostarczenie wszystkich ostrzeżeń, w tym ostrzeżeń, które nie zawsze są przydatne, takich jak ostrzeżenia stylistyczne. (Osobiście wybieram to ostrzeżenie, ponieważ nie chcę wartości domyślnych w moich przełącznikach wyliczania; posiadanie ich oznacza, że ​​nie otrzymuję ostrzeżenia, jeśli dodam wartość wyliczenia i nie obsługuję jej. Z tego powodu zawsze obsługuję przypadek złej wartości poza wyliczeniem.)
Jens Ayton
2
@JensAyton Jest zdezorientowany. Wszystkie profesjonalne narzędzia do analizowania statycznego, a także wszystkie kompilatory zgodne z MISRA będą wyświetlać ostrzeżenie, jeśli instrukcja switch nie ma instrukcji domyślnej. Spróbuj sam.
4
Kodowanie do MISRA-C nie jest jedynym prawidłowym zastosowaniem kompilatora C i ponownie -Weverythingwłącza każde ostrzeżenie, a nie wyborem odpowiednim dla konkretnego przypadku użycia. Nie pasowanie do gustu nie jest tym samym, co zamieszanie.
Jens Ayton
2
@Lundin: Jestem prawie pewien, że trzymanie się MISRA-C nie magicznie czyni programów bezpiecznymi i wolnymi od błędów ... i jestem równie pewien, że jest mnóstwo bezpiecznego, wolnego od błędów kodu C od ludzi, którzy nigdy nie mieli słyszałem o MISRA. W rzeczywistości jest to niewiele więcej niż czyjaś (popularna, ale nie niekwestionowana) opinia, a są ludzie, którzy uważają niektóre jej zasady za arbitralne, a czasem nawet szkodliwe poza kontekstem, dla którego zostały zaprojektowane.
cHao 14.12.12
4

Jeszcze lepiej:

typedef enum {
    MaskValueUno,
    MaskValueDos,

    MaskValue_count
} testingMask;

void myFunction(testingMask theMask) {
    assert(theMask >= 0 && theMask<MaskValue_count);
    switch theMask {
        case MaskValueUno: {}// deal with it
        case MaskValueDos: {}// deal with it
    }
};

Jest to mniej podatne na błędy podczas dodawania elementów do wyliczenia. Możesz pominąć test dla> = 0, jeśli twoje wartości wyliczone są niepodpisane. Ta metoda działa tylko wtedy, gdy nie ma luk w wartościach wyliczeniowych, ale często tak jest.

Steve Weller
źródło
2
To zanieczyszcza typ wartościami, których po prostu nie chcesz, aby zawierał typ. Ma podobieństwa ze starym dobrym WTF tutaj: thedailywtf.com/articles/What_Is_Truth_0x3f_
Martin Sugioarto
4

Ale co jeśli jakiś hack pojawi się i wrzuci niezainicjowaną int do mojej pięknej funkcji?

Wtedy otrzymasz niezdefiniowane zachowanie i swojedefault wola będzie bez znaczenia. Nic nie możesz zrobić, aby to poprawić.

Pozwól mi być bardziej jasne. Gdy tylko ktoś przejdzie intw twoją funkcję niezainicjalizowany , stanie się niezdefiniowanym. Twoja funkcja może rozwiązać problem zatrzymania i nie ma znaczenia. To jest UB. Po wywołaniu UB nie można nic zrobić.

DeadMG
źródło
3
Co powiesz na logowanie lub rzucenie wyjątku, jeśli po cichu go zignorujesz?
jgauffin
3
Rzeczywiście, można wiele zrobić, na przykład ... dodać domyślną instrukcję do przełącznika i z wdzięcznością obsłużyć błąd. Nazywa się to programowaniem obronnym . Chroni nie tylko przed głupim programistą podającym nieprawidłowe dane wejściowe, ale także przed różnymi błędami spowodowanymi przepełnieniem bufora, przepełnieniem stosu, niekontrolowanym kodem itp.
1
Nie, nic nie poradzi. To jest UB. Program ma UB od momentu wejścia do funkcji, a ciało funkcji nic na to nie poradzi.
DeadMG,
2
@DeadMG Wyliczenie może mieć dowolną wartość, jaką może mieć odpowiadający jej typ całkowity w implementacji. Nie ma w tym nic niezdefiniowanego. Dostęp do zawartości niezainicjowanej zmiennej automatycznej jest rzeczywiście UB, ale „hack zła” (który jest bardziej prawdopodobnym błędem) może również rzucić dobrze zdefiniowaną wartość do funkcji, nawet jeśli nie jest to jedna z wartości wymienione w deklaracji wyliczeniowej.
2

Domyślna instrukcja niekoniecznie pomoże. Jeśli przełącznik znajduje się nad wyliczeniem, każda wartość, która nie jest zdefiniowana w wyliczeniu, spowoduje niezdefiniowane zachowanie.

Z tego co wiesz, kompilator może skompilować ten przełącznik (z ustawieniem domyślnym) jako:

if (theMask == MaskValueUno)
  // Execute something MaskValueUno code
else // theMask == MaskValueDos
  // Execute MaskValueDos code

Po uruchomieniu nieokreślonego zachowania nie ma już powrotu.

Siebie
źródło
2
Po pierwsze, jest to nieokreślona wartość , a nie niezdefiniowane zachowanie. Oznacza to, że kompilator może przyjąć inną wygodniejszą wartość, ale może nie zmienić księżyca w zielony ser itp. Po drugie, o ile wiem, dotyczy to tylko C ++. W C zakres wartości enumtypu jest taki sam, jak zakres wartości leżącego u jego podstaw typu liczb całkowitych (który jest zdefiniowany w implementacji, dlatego musi być spójny).
Jens Ayton
1
Jednak wystąpienie zmiennej wyliczeniowej i stałej wyliczenia niekoniecznie musi mieć tę samą szerokość i sygnaturę, oba są domyślnie zdefiniowane. Ale jestem prawie pewien, że wszystkie wyliczenia w C są oceniane dokładnie tak, jak odpowiadający im typ liczb całkowitych, więc nie ma tam niezdefiniowanego lub nawet nieokreślonego zachowania.
2

Ja też wolę mieć default:we wszystkich przypadkach. Jak zwykle spóźniam się na przyjęcie, ale ... kilka innych myśli, których nie widziałem powyżej:

  • Szczególne ostrzeżenie (lub błąd, jeśli także rzucanie -Werror) pochodzi od -Wcovered-switch-default(z, -Weverythingale nie -Wall). Jeśli twoja moralna elastyczność pozwala ci wyłączyć niektóre ostrzeżenia (tj. Wyciąć niektóre rzeczy z -Walllub -Weverything), rozważ rzucanie -Wno-covered-switch-default(lub -Wno-error=covered-switch-defaultpodczas używania -Werror), i ogólnie -Wno-...dla innych ostrzeżeń, które uważasz za nieprzyjemne.
  • Do gcc(i bardziej ogólne zachowanie w clang), skonsultować gccmanpage dla -Wswitch, -Wswitch-enum, -Wswitch-defaultdla (różnych) zachowanie w podobnych sytuacjach wymienionych typów w sprawozdaniu przełącznika.
  • Nie podoba mi się też to ostrzeżenie w koncepcji i nie podoba mi się jego brzmienie; dla mnie słowa z ostrzeżenia („etykieta domyślna ... obejmuje wszystkie ... wartości”) sugerują, że default:sprawa będzie zawsze wykonywana, na przykład

    switch (foo) {
      case 1:
        do_something(); 
        //note the lack of break (etc.) here!
      default:
        do_default();
    }

    Przy pierwszym czytaniu myślałem, że na to wpadłeś - że twoja default:sprawa będzie zawsze wykonywana, ponieważ nie ma break;lub nie ma return;podobnego. Ta koncepcja jest podobna (do mojego ucha) do innych komentarzy w stylu niani (choć czasami pomocnych), z których wyłania się clang. Jeśli foo == 1obie funkcje zostaną wykonane; powyższy kod ma takie zachowanie. Tj. Nie przełamuj się tylko wtedy, gdy chcesz nadal wykonywać kod z kolejnych przypadków! Wydaje się to jednak nie być twoim problemem.

Ryzykując pedantyczne, kilka innych myśli o kompletności:

  • Myślę jednak, że takie zachowanie jest (bardziej) spójne z agresywnym sprawdzaniem typów w innych językach lub kompilatorach. Jeżeli, jak to hipotezę, niektóre potępiony nie próbować przekazać intlub coś do tej funkcji, która jest wyraźnie zamierzający konsumować własny typ specyficzny, kompilator powinien zabezpieczyć się równie dobrze w tej sytuacji z agresywnym ostrzeżenia lub błędu. Ale tak nie jest! (To znaczy, wydaje się, że co najmniej gcci clangnie rób enumtypu sprawdzania, ale słyszę, że iccrobi ). Ponieważ nie otrzymujesz bezpieczeństwa typu , możesz uzyskać bezpieczeństwo wartości, jak omówiono powyżej. W przeciwnym razie, jak zasugerowano w TFA, zastanów się nad structczymś, co może zapewnić bezpieczeństwo typu.
  • Innym obejściem może być tworzenie nowej „wartości” w twoim, na enumprzykład MaskValueIllegal, i nie wspieranie tego casew twoim switch. Zostałby zjedzony przez default:(oprócz każdej innej zwariowanej wartości)

Niech żyje kodowanie obronne!

hoc_age
źródło
1

Oto alternatywna sugestia:
OP stara się chronić przed przypadkiem, gdy ktoś przechodzi w miejscu, w intktórym spodziewany jest wyliczenie . Lub, bardziej prawdopodobne, gdy ktoś połączył starą bibliotekę z nowszym programem przy użyciu nowszego nagłówka z większą liczbą przypadków.

Dlaczego nie zmienić przełącznika do obsługi intskrzynki? Dodanie rzutowania przed wartością w przełączniku eliminuje ostrzeżenie, a nawet daje pewien podpowiedź, dlaczego istnieje wartość domyślna.

void myFunction(testingMask theMask) {
    int maskValue = int(theMask);
    switch(maskValue) {
        case MaskValueUno: {} // deal with it
        case MaskValueDos: {}// deal with it
        default: {} //deal with an unexpected or uninitialized value
    }
}

Uważam, że jest to o wiele mniej budzące zastrzeżenia niż assert()testowanie każdej z możliwych wartości, a nawet przyjmowanie założenia, że ​​zakres wartości wyliczeniowych jest dobrze uporządkowany, dzięki czemu działa prostszy test. Jest to po prostu brzydki sposób robienia tego, co domyślne robi dokładnie i pięknie.

Richard
źródło
1
ten post jest raczej trudny do odczytania (ściana tekstu). Czy mógłbyś edytować go w lepszym kształcie?
komnata