Czy rozsądnie jest strzec zerowania każdego pojedynczego, pozbawionego odniesień wskaźnika?

65

Przy nowej pracy dostaję oznaczenie w recenzjach kodu dla kodu w następujący sposób:

PowerManager::PowerManager(IMsgSender* msgSender)
  : msgSender_(msgSender) { }

void PowerManager::SignalShutdown()
{
    msgSender_->sendMsg("shutdown()");
}

Powiedziano mi, że ostatnia metoda powinna brzmieć:

void PowerManager::SignalShutdown()
{
    if (msgSender_) {
        msgSender_->sendMsg("shutdown()");
    }
}

tzn. muszę ustawić NULLosłonę wokół msgSender_zmiennej, nawet jeśli jest ona prywatnym członkiem danych. Trudno mi powstrzymać się od użycia przekleństw, aby opisać, co sądzę o tym „mądrości”. Kiedy proszę o wyjaśnienie, dostaję litanię horrorów o tym, jak jakiś młodszy programista, pewnego roku, pomylił się co do tego, jak klasa powinna działać, i przypadkowo usunął członka, którego nie powinien był (i ustawił NULLpóźniej , najwyraźniej), a rzeczy wybuchły w terenie zaraz po wydaniu produktu, a my „nauczyliśmy się twardej drogi, zaufaj nam”, że lepiej po prostu wszystkoNULL sprawdzić .

Dla mnie jest to programowanie z kultowym ładunkiem , proste i proste. Kilku współpracowników, którzy mają dobre intencje, poważnie stara się pomóc mi „zdobyć” i zobaczyć, jak to pomoże mi napisać bardziej solidny kod, ale ... Nie mogę się powstrzymać od poczucia, że ​​to oni go nie rozumieją .

Czy rozsądne jest, aby standard kodowania wymagał, aby każdy wskaźnik odsunięty od funkcji w NULLpierwszej kolejności był sprawdzany w pierwszej kolejności - nawet dla prywatnych członków danych? (Uwaga: aby nadać kontekstu, tworzymy elektronikę użytkową, a nie system kontroli ruchu lotniczego lub inny produkt „brak równości ludzi”).

EDYCJA : W powyższym przykładzie msgSender_współpracownik nie jest opcjonalny. Jeśli kiedykolwiek NULL, oznacza to błąd. Jedynym powodem, dla którego jest przekazywany do konstruktora, jest PowerManagertestowanie IMsgSenderpodklasy próbnej.

STRESZCZENIE : Dziękuję wszystkim za naprawdę świetne odpowiedzi na to pytanie. Zaakceptowałem ten z @aaronps głównie ze względu na jego zwięzłość. Wydaje się, że istnieje ogólna zgoda co do tego, że:

  1. Nakazanie NULLstrażnikom każdego odsuniętego wskaźnika jest przesadą, ale
  2. Możesz przejść całą debatę na bok, używając zamiast tego odwołania (jeśli to możliwe) lub constwskaźnika i
  3. assertinstrukcje są bardziej oświeconą alternatywą dla NULLstrażników do sprawdzania, czy spełnione są warunki wstępne funkcji.
evadeflow
źródło
14
Nie myśl przez chwilę, że błędy są domeną młodszych programistów. 95% programistów na wszystkich poziomach doświadczenia raz na jakiś czas coś robi. Pozostałe 5% leży przez zęby.
Blrfl
56
Gdybym widział, jak ktoś pisze kod, który sprawdza wartość zerową i po cichu zawiedzie, chciałbym go zwolnić na miejscu.
Winston Ewert
16
Ciche zawodzenie rzeczy jest prawie najgorsze. Przynajmniej kiedy wybuchnie, wiesz, gdzie ma miejsce eksplozja. Sprawdzanie nulli nic nie robienie jest tylko sposobem na przeniesienie błędu w dół wykonania, co znacznie utrudnia powrót do źródła.
MirroredFate
1
+1, bardzo interesujące pytanie. Oprócz mojej odpowiedzi wskazałbym również, że kiedy trzeba napisać kod, którego celem jest przeprowadzenie testu jednostkowego, to tak naprawdę robimy handel zwiększonym ryzykiem fałszywego poczucia bezpieczeństwa (więcej wierszy kodu = więcej możliwości błędów). Przeprojektowanie w celu użycia referencji nie tylko poprawia czytelność kodu, ale także zmniejsza ilość kodu (i testów), które musisz napisać, aby udowodnić, że działa.
Seth
6
@Rig, jestem pewien, że istnieją odpowiednie przypadki cichej awarii. Ale jeśli ktoś cicho zawodzi jako ogólną praktykę (chyba że praca w dziedzinie, w której ma to sens), naprawdę nie chcę pracować nad projektem, w który wpisują kod.
Winston Ewert

Odpowiedzi:

66

To zależy od „umowy”:

Jeśli PowerManager MUSI mieć prawidłową wartość IMsgSender, nigdy nie sprawdzaj wartości zerowej, pozwól jej umrzeć wcześniej.

Jeśli z drugiej strony, MOGĄ mieć IMsgSender, to musisz sprawdzić za każdym razem, tak proste.

Końcowy komentarz na temat historii młodszego programisty, problemem jest w rzeczywistości brak procedur testowych.

aaronps
źródło
4
+1 za bardzo zwięzłą odpowiedź, która właściwie podsumowuje to, co czuję: „to zależy ...” Miałeś też odwagę powiedzieć „nigdy nie sprawdzaj null” ( jeśli null jest nieprawidłowy). Umieszczenie funkcji assert()w każdym elemencie, która nie uwzględnia wskaźnika, jest kompromisem, który prawdopodobnie uspokoi wiele osób, ale nawet to wydaje mi się „spekulatywną paranoją”. Lista rzeczy, nad którymi nie mam kontroli, jest nieskończona, a kiedy pozwolę sobie zacząć się martwić o nie, staje się naprawdę śliska. Nauka zaufania do moich testów, moi koledzy - i mój debugger - pomogli mi lepiej spać w nocy.
evadeflow,
2
Podczas czytania kodu te stwierdzenia mogą być bardzo pomocne w zrozumieniu, w jaki sposób kod powinien działać. Nigdy nie natknąłem się na bazę kodów, która skłoniła mnie do bycia „Chciałbym, żeby nie było tam tych wszystkich twierdzeń”, ale widziałem wiele, gdzie powiedziałem coś przeciwnego.
Kristof Provost
1
Prosta i prosta, to poprawna odpowiedź na pytanie.
Matthew Azkimov
2
To jest najbliższa poprawna odpowiedź, ale nie mogę się zgodzić z „nigdy nie sprawdzaj null”, zawsze sprawdzaj nieprawidłowe parametry w punkcie, w którym zostaną przypisane do zmiennej członka.
James
Dziękuję za komentarze. Lepiej pozwolić, aby zawiesił się wcześniej, jeśli ktoś nie przeczyta specyfikacji i zainicjuje ją wartością zerową, gdy musi tam być coś ważnego.
aaronps,
77

Wydaje mi się, że kod powinien brzmieć:

PowerManager::PowerManager(IMsgSender* msgSender)
  : msgSender_(msgSender)
{
    assert(msgSender);
}

void PowerManager::SignalShutdown()
{
    assert(msgSender_);
    msgSender_->sendMsg("shutdown()");
}

Jest to w rzeczywistości lepsze niż pilnowanie NULL, ponieważ wyraźnie pokazuje, że funkcja nigdy nie powinna być wywoływana, jeśli msgSender_jest NULL. Daje również pewność, że zauważysz, jeśli tak się stanie.

Wspólna „mądrość” twoich kolegów po cichu zignoruje ten błąd, z nieprzewidywalnymi rezultatami.

Ogólnie błędy są łatwiejsze do naprawienia, jeśli zostaną wykryte bliżej ich przyczyny. W tym przykładzie proponowana ochrona NULL doprowadziłaby do braku ustawiania komunikatu zamknięcia, co może, ale nie musi, powodować zauważalny błąd. Trudniej byłoby pracować wstecz do tej SignalShutdownfunkcji, niż gdyby cała aplikacja właśnie umarła, tworząc poręczny elegancki ślad lub zrzut rdzenia wskazujący bezpośrednio na SignalShutdown().

Jest to trochę sprzeczne z intuicją, ale zawieszanie się, gdy tylko coś jest nie tak, sprawia, że ​​kod jest bardziej niezawodny. Wynika to z faktu, że faktycznie znajdują się problemy i mają one również bardzo oczywiste przyczyny.

Kristof Provost
źródło
10
Duża różnica między wywoływaniem asertu a przykładem kodu PO polega na tym, że aser jest włączony tylko w kompilacjach debugowania (# zdefiniować NDEBUG) Gdy produkt trafi w ręce klienta, a debugowanie jest wyłączone, a nigdy nie sprawdzona kombinacja ścieżek kodu jest wykonywana, aby wyjść wskaźnik jest pusty, nawet jeśli funkcja jest wykonywana, aser nie zapewnia żadnej ochrony.
atk
17
Prawdopodobnie przetestowałeś kompilację przed wysłaniem jej do klienta, ale tak, to możliwe ryzyko. Rozwiązania są jednak łatwe: albo po prostu wysyłaj z włączonymi asertami (to i tak testowana wersja) lub zamień je na własne makro, które zapewnia w trybie debugowania i tylko loguje, loguje + ślady wsteczne, wychodzi, ... w trybie wydania.
Kristof Provost
7
@James - w 90% przypadków nie wyzdrowiejesz również z nieudanego sprawdzenia NULL. Ale w takim przypadku kod po cichu zawiedzie, a błąd może pojawić się znacznie później - na przykład myślałeś, że zapisałeś do pliku, ale w rzeczywistości kontrola zerowa czasami kończy się niepowodzeniem, pozostawiając cię mądrzejszym. Nie możesz wyzdrowieć ze wszystkich sytuacji, które nie mogą się zdarzyć. Możesz sprawdzić, czy tak się nie stanie, ale nie sądzę, że masz budżet, chyba że napiszesz kod dla satelity NASA lub elektrowni jądrowej. Musisz umieć powiedzieć: „Nie mam pojęcia, co się dzieje - program nie powinien być w tym stanie”.
Maciej Piechotka,
6
@James Nie zgadzam się z rzucaniem. To sprawia, że ​​wygląda na to, że może się zdarzyć. Aserty dotyczą rzeczy, które powinny być niemożliwe. Innymi słowy, dla prawdziwych błędów w kodzie. Wyjątek stanowią przypadki niespodziewane, ale możliwe. Wyjątek stanowią rzeczy, które można sobie wyobrazić i odzyskać. Błędy logiczne w kodzie, z którego nie można odzyskać, i nie powinieneś również próbować.
Kristof Provost
2
@James: Z mojego doświadczenia z systemami wbudowanymi, oprogramowanie i sprzęt są niezmiennie zaprojektowane tak, że bardzo trudno jest uczynić urządzenie całkowicie bezużytecznym. W zdecydowanej większości przypadków istnieje sprzętowy organ nadzorczy, który wymusza całkowity reset urządzenia, jeśli oprogramowanie przestanie działać.
Bart van Ingen Schenau
30

Jeśli msgSender nie musi być null, należy umieścić nullsprawdzanie tylko w konstruktorze. Odnosi się to również do wszelkich innych danych wejściowych do klasy - umieść kontrolę integralności w punkcie wejścia do „modułu” - klasa, funkcja itp.

Moją ogólną zasadą jest sprawdzanie integralności między granicami modułów - w tym przypadku klasą. Ponadto klasa powinna być na tyle mała, aby możliwe było szybkie mentalne zweryfikowanie integralności czasów życia członków klasy - zapewniając, że zapobiegnie się takim błędom, jak niewłaściwe usuwanie / zerowanie przypisań. Kontrola zerowa przeprowadzana w kodzie w poście zakłada, że ​​każde nieprawidłowe użycie faktycznie przypisuje null do wskaźnika - co nie zawsze ma miejsce. Ponieważ „nieprawidłowe użycie” z natury oznacza, że ​​nie mają zastosowania żadne założenia dotyczące zwykłego kodu, nie możemy być pewni, że wychwycimy wszystkie rodzaje błędów wskaźnika - na przykład nieprawidłowe usunięcie, przyrost itp.

Ponadto - jeśli masz pewność, że argument nigdy nie może być pusty, rozważ użycie referencji, w zależności od użycia klasy. W przeciwnym razie rozważ użycie surowego wskaźnika std::unique_ptrlub std::shared_ptrzamiast niego.

Max
źródło
11

Nie, sprawdzanie każdego odwołania do wskaźnika dla istoty wskaźnika nie jest rozsądne NULL.

Kontrole zerowe są cenne dla argumentów funkcji (w tym argumentów konstruktora), aby zapewnić spełnienie warunków wstępnych lub podjąć odpowiednie działania, jeśli nie podano opcjonalnego parametru, i są one przydatne do sprawdzenia niezmiennika klasy po odsłonięciu wewnętrznych elementów klasy. Ale jeśli jedynym powodem, dla którego wskaźnik ma wartość NULL, jest istnienie błędu, wówczas sprawdzanie nie ma sensu. Ten błąd równie łatwo mógł ustawić wskaźnik na inną niepoprawną wartość.

Gdybym miał do czynienia z taką sytuacją, zadałbym dwa pytania:

  • Dlaczego błąd tego młodszego programisty nie został wykryty przed wydaniem? Na przykład podczas przeglądu kodu lub w fazie testowania? Wprowadzenie wadliwego urządzenia elektroniki użytkowej na rynek może być równie kosztowne (jeśli weźmie się pod uwagę udział w rynku i dobrą wolę), jak wydanie wadliwego urządzenia używanego w krytycznych dla bezpieczeństwa aplikacjach, więc oczekiwałbym, że firma poważnie podchodzi do testowania i inne działania związane z kontrolą jakości.
  • Jeśli sprawdzanie wartości zerowej nie powiedzie się, jakiej obsługi błędów oczekuje się ode mnie, czy mógłbym po prostu napisać assert(msgSender_)zamiast kontroli wartości zerowej? Jeśli po prostu włączysz sprawdzanie wartości zerowej, mógłbyś zapobiec awarii, ale mogłeś stworzyć gorszą sytuację, ponieważ oprogramowanie kontynuuje działanie przy założeniu, że operacja miała miejsce, podczas gdy w rzeczywistości operacja ta została pominięta. Może to spowodować niestabilność innych części oprogramowania.
Bart van Ingen Schenau
źródło
9

Ten przykład wydaje się bardziej dotyczyć czasu życia obiektu niż tego, czy parametr wejściowy ma wartość null †. Ponieważ wspominasz, że zawszePowerManager musi mieć poprawny , przekazanie argumentu przez wskaźnik (umożliwiając w ten sposób możliwość zerowego wskaźnika) wydaje mi się błędem projektowym ††.IMsgSender

W takich sytuacjach wolałbym zmienić interfejs, aby wymagania osoby dzwoniącej były egzekwowane przez język:

PowerManager::PowerManager(const IMsgSender& msgSender)
  : msgSender_(msgSender) {}

void PowerManager::SignalShutdown() {
    msgSender_->sendMsg("shutdown()");
}

Przepisanie tego w ten sposób mówi, że PowerManagernależy zachować odniesienie IMsgSenderprzez cały okres jego istnienia. To z kolei ustanawia również dorozumiany wymóg, który IMsgSendermusi istnieć dłużej niż PowerManager, i neguje potrzebę jakiegokolwiek sprawdzania wskaźnika zerowego lub stwierdzeń w środku PowerManager.

Możesz również napisać to samo za pomocą inteligentnego wskaźnika (poprzez boost lub c ++ 11), aby jawnie wymusić IMsgSenderżycie dłuższe niż PowerManager:

PowerManager::PowerManager(std::shared_ptr<IMsgSender> msgSender) 
  : msgSender_(msgSender) {}

void PowerManager::SignalShutdown() {
    // Here, we own a smart pointer to IMsgSender, so even if the caller
    // destroys the original pointer, we still have a valid copy
    msgSender_->sendMsg("shutdown()");
}

Ta metoda jest preferowana, jeśli jest możliwe, że IMsgSendernie można zagwarantować, że jej żywotność będzie dłuższa niż PowerManager(tj x = new IMsgSender(); p = new PowerManager(*x);.).

† W odniesieniu do wskaźników: gwałtowne sprawdzanie wartości null sprawia, że ​​kod jest trudniejszy do odczytania i nie poprawia stabilności (poprawia wygląd stabilności, co jest znacznie gorsze).

Gdzieś ktoś ma adres do przechowywania pamięci IMsgSender. Zadaniem tej funkcji jest upewnienie się, że alokacja się powiodła (sprawdzenie wartości zwracanych przez bibliotekę lub poprawna obsługa std::bad_allocwyjątków), aby nie przekazywać niepoprawnych wskaźników.

Ponieważ PowerManagernie jest właścicielem IMsgSender(tylko pożycza go na jakiś czas), nie jest odpowiedzialny za przydzielanie ani niszczenie tej pamięci. To kolejny powód, dla którego wolę referencję.

†† Ponieważ jesteś nowy w tej pracy, oczekuję, że włamujesz się do istniejącego kodu. Więc z wady projektowej mam na myśli, że wada jest w kodzie, z którym pracujesz. Dlatego osoby, które oflagują Twój kod, ponieważ nie sprawdza zerowych wskaźników, tak naprawdę same się zgłaszają do pisania kodu, który wymaga wskaźników :)

Seth
źródło
1
Czasami nie ma nic złego w korzystaniu z surowych wskaźników. Std :: shared_ptr nie jest srebrną kulą, a użycie go na liście argumentów jest lekarstwem na niechlujną inżynierię, chociaż zdaję sobie sprawę, że używanie go, gdy jest to możliwe, jest uważane za „leet”. Użyj java, jeśli masz na to ochotę.
James
2
Nie powiedziałem, że surowe wskazówki są złe! Zdecydowanie mają swoje miejsce. Jednak jest to C + + , referencje (i wspólne wskaźniki, teraz) są częścią języka powodu. Użyj ich, sprawią, że odniesiesz sukces.
Seth
Podoba mi się pomysł inteligentnego wskaźnika, ale nie jest to opcja na tym konkretnym koncercie. +1 za zalecenie użycia referencji (tak jak @Max i kilka innych). Czasami nie jest możliwe, aby wprowadzić odniesienie, chociaż, ze względu na problemy z zależnościami kurczaka i jaj, czyli jeśli obiekt, który inaczej byłby kandydatem do wstrzykiwań musi mieć wskaźnik (lub odniesienia) do uchwytu wtryskiwanego do niej . Może to czasem wskazywać na ściśle powiązany projekt; ale często jest to akceptowalne, jak w przypadku relacji między obiektem a jego iteratorem, gdzie nigdy nie ma sensu używać tego drugiego bez tego pierwszego.
evadeflow,
8

Podobnie jak wyjątki, warunki ochrony są użyteczne tylko wtedy, gdy wiesz, co zrobić, aby naprawić błąd lub jeśli chcesz przekazać bardziej znaczący komunikat wyjątku.

Połknięcie błędu (bez względu na to, czy został złapany jako wyjątek, czy kontrola bezpieczeństwa) jest właściwą rzeczą, gdy błąd nie ma znaczenia. Najczęstsze miejsce, w którym widzę błędy podczas połykania, to kod rejestrowania błędów - nie chcesz zawieszać aplikacji, ponieważ nie możesz zalogować się do komunikatu o stanie.

Za każdym razem, gdy wywoływana jest funkcja i nie jest to zachowanie opcjonalne, powinna zawieść głośno, nie po cichu.

Edycja: myśląc o twojej historii młodszego programisty, brzmi to tak, jakby zdarzyło się, że prywatny członek został ustawiony na zero, kiedy to nigdy nie powinno było mieć miejsca. Mieli problem z niewłaściwym pisaniem i próbują to naprawić, sprawdzając poprawność podczas czytania. To jest wstecz. Zanim go zidentyfikujesz, błąd już się pojawił. Egzekwowalne rozwiązanie do sprawdzania kodu / kompilatora nie stanowi warunków ochrony, ale pobiera i ustawia lub członków stałych.

jmoreno
źródło
7

Jak zauważyli inni, zależy to od tego, czy msgSendermoże być legalne NULL . Poniżej założono, że nigdy nie powinno być NULL.

void PowerManager::SignalShutdown()
{
    if (!msgSender_)
    {
       throw SignalException("Shut down failed because message sender is not set.");
    }

    msgSender_->sendMsg("shutdown()");
}

Proponowana „poprawka” przez innych członków zespołu narusza zasadę Dead Programs Tell No Lies . Błędy są naprawdę trudne do znalezienia. Metoda, która dyskretnie zmienia swoje zachowanie na podstawie wcześniejszego problemu, nie tylko utrudnia znalezienie pierwszego błędu, ale także dodaje drugi błąd.

Młodzieniec siał spustoszenie, nie sprawdzając, czy ma wartość zerową. Co się stanie, jeśli ten fragment kodu spowoduje spustoszenie, kontynuując działanie w nieokreślonym stanie (urządzenie jest włączone, ale program „myśli”, że jest wyłączone)? Być może inna część programu zrobi coś, co jest bezpieczne tylko wtedy, gdy urządzenie jest wyłączone.

Każde z tych podejść pozwoli uniknąć cichych awarii:

  1. Używaj asercji zgodnie z sugestią podaną w tej odpowiedzi , ale upewnij się, że są włączone w kodzie produkcyjnym. Mogłoby to oczywiście powodować problemy, gdyby inne twierdzenia zostały napisane przy założeniu, że zostaną wyłączone z produkcji.

  2. Zgłaszaj wyjątek, jeśli jest on pusty.

szturchać
źródło
5

Zgadzam się z zatrzymywaniem wartości null w konstruktorze. Ponadto, jeśli element członkowski jest zadeklarowany w nagłówku jako:

IMsgSender* const msgSender_;

Wówczas wskaźnika nie można zmienić po inicjalizacji, więc jeśli był w porządku podczas budowy, będzie w porządku przez cały okres życia obiektu zawierającego go. (Obiekt zwrócił się będzie nie być const).

Grimm Opiner
źródło
To świetna rada, dziękuję! Tak dobrze, że tak mi przykro, że nie mogę głosować wyżej. Nie jest to bezpośrednia odpowiedź na zadane pytanie, ale jest to świetny sposób na wyeliminowanie jakiejkolwiek możliwości przypadkowego stania się wskaźnika NULL.
evadeflow,
@evadeflow Myślałem, że „zgadzam się ze wszystkimi”, odpowiedział na główny wątek pytania. Na zdrowie! ;-)
Grimm The Opiner
Byłoby trochę niegrzeczne z mojej strony, aby zmienić zaakceptowaną odpowiedź w tym miejscu, biorąc pod uwagę sposób sformułowania pytania. Ale naprawdę uważam, że ta rada jest znakomita i przepraszam, że sama o tym nie pomyślałam. Ze constwskaźnikiem nie ma potrzeby assert()wychodzenia poza konstruktor, więc ta odpowiedź wydaje się nawet nieco lepsza niż @Kristof Provost (który był również wybitny, ale rozwiązał to pytanie również pośrednio). Mam nadzieję, że inni głosują na to, ponieważ to naprawdę nie ma sensu assertw każdej metodzie, kiedy można po prostu zrobić wskaźnik const.
evadeflow,
@evadeflow Ludzie odwiedzają Questins tylko dla przedstawicieli, teraz otrzymano odpowiedź, że nikt więcej na nie nie spojrzy. ;-) Wpadłem tylko dlatego, że było to pytanie o wskaźniki i uważam na cholerną brygadę „Używaj inteligentnych wskaźników do wszystkiego”.
Grimm The Opiner
3

To jest całkowicie niebezpieczne!

Pracowałem pod kierownictwem starszego programisty w bazie kodu C z najbardziej podejrzanymi „standardami”, którzy nalegali na to samo, aby ślepo sprawdzać wszystkie wskaźniki pod kątem wartości zerowej. Deweloper ostatecznie zrobiłby takie rzeczy:

// Pre: vertex should never be null.
void transform_vertex(Vertex* vertex, ...)
{
    // Inserted by my "wise" co-worker.
    if (!vertex)
        return;
    ...
}

Kiedyś próbowałem usunąć taką kontrolę warunku raz w takiej funkcji i zastąpić ją, assertaby zobaczyć, co się stanie.

Ku mojemu przerażeniu znalazłem tysiące linii kodu w bazie kodu, które przekazywały wartości zerowe do tej funkcji, ale gdzie programiści, prawdopodobnie zdezorientowani, pracowali i dodawali więcej kodu, aż wszystko zadziałało.

Ku mojemu dalszemu przerażeniu odkryłem, że problem ten występuje we wszystkich miejscach w bazie kodów, sprawdzając, czy nie ma żadnych wartości zerowych. Baza kodów rozrosła się przez dziesięciolecia, aby polegać na tych kontrolach, aby móc po cichu naruszać nawet najbardziej udokumentowane warunki wstępne. Usuwając te śmiertelne kontrole na korzyść asserts, ujawnią się wszystkie logiczne błędy ludzkie w ciągu dziesięcioleci w bazie kodu, a my się w nich utopimy.

Potrzebne były tylko dwa pozornie niewinne wiersze kodu, takie jak ten + czas, i zespół, aby zamaskować tysiąc nagromadzonych błędów.

Są to rodzaje praktyk, które uzależniają błędy od innych błędów, aby oprogramowanie działało. To scenariusz koszmaru . Sprawia również, że każdy błąd logiczny związany z naruszeniem takich warunków wstępnych pokazuje tajemniczo milion wierszy kodu z dala od rzeczywistej strony, w której wystąpił błąd, ponieważ wszystkie te testy zerowe po prostu ukrywają błąd i ukrywają go, aż dotrzemy do miejsca, które zapomniało aby ukryć błąd.

Po prostu ślepe sprawdzanie wartości zerowych w każdym miejscu, w którym wskaźnik zerowy narusza warunek wstępny, jest dla mnie całkowitym szaleństwem, chyba że twoje oprogramowanie jest tak krytyczne pod względem misji w przypadku stwierdzeń i awarii produkcyjnych, że potencjał tego scenariusza jest lepszy.

Czy rozsądne jest, aby standard kodowania wymagał, aby każdy wskaźnik odsunięty od funkcji w pierwszej kolejności był sprawdzany pod kątem NULL - nawet prywatnych członków danych?

Więc powiedziałbym, absolutnie nie. To nawet nie jest „bezpieczne”. Może być odwrotnie i maskować wszelkiego rodzaju błędy w bazie kodu, które z biegiem lat mogą prowadzić do najbardziej przerażających scenariuszy.

assertjest drogą do przejścia tutaj. Naruszenie warunków wstępnych nie powinno pozostać niezauważone, w przeciwnym razie prawo Murphy'ego może się łatwo rozpocząć.


źródło
1
„assert” jest właściwą drogą - pamiętaj jednak, że w każdym nowoczesnym systemie każdy dostęp do pamięci za pośrednictwem wskaźnika ma wbudowany w sprzęcie aser wskaźnik zerowy. Tak więc w „assert (p! = NULL); return * p;” nawet assert jest w większości bezcelowy.
gnasher729
@ gnasher729 Ah, to bardzo dobra uwaga, ale zwykle postrzegam assertzarówno jako mechanizm dokumentacji, aby ustalić, jakie są warunki wstępne, a nie tylko sposób na wymuszenie przerwania. Ale podoba mi się również rodzaj błędu asercji, który pokazuje dokładnie, który wiersz kodu spowodował awarię, a warunek asercji nie powiódł się („dokumentowanie awarii”). Może się przydać, jeśli w końcu użyjemy kompilacji debugowania poza debuggerem i zostaniemy zaskoczeni.
@ gnasher729 Albo mogłem źle zrozumieć jego część. Czy te nowoczesne systemy pokazują, w której linii kodu źródłowego nie udało się potwierdzić? Generalnie wyobrażam sobie, że w tym momencie stracili informacje i mogą po prostu pokazać segfault / naruszenie dostępu, ale jestem trochę daleki od „nowoczesności” - nadal uparcie na Windows 7 przez większość mojego rozwoju.
Na przykład w systemach MacOS X i iOS, jeśli aplikacja ulegnie awarii z powodu braku dostępu do wskaźnika podczas programowania, debugger wyświetli dokładną linię kodu w miejscu, w którym ma to miejsce. Jest jeszcze jeden dziwny aspekt: ​​kompilator spróbuje ostrzec cię, jeśli zrobisz coś podejrzanego. Jeśli przekażesz wskaźnik do funkcji i uzyskasz do niej dostęp, kompilator nie wyświetli ostrzeżenia, że ​​wskaźnik może mieć wartość NULL, ponieważ zdarza się to tak często, że utopiłbyś się w ostrzeżeniach. Jeśli jednak wykonasz porównanie, jeśli (p == NULL) ... to kompilator zakłada, że ​​istnieje
spora
ponieważ dlaczego miałbyś go przetestować inaczej, dlatego wyświetli ostrzeżenie, jeśli będziesz go używać bez testowania. Jeśli używasz własnego makra przypominającego asercję, musisz być trochę sprytny, aby napisać go w taki sposób, aby „my_assert (p! = NULL,„ To wskaźnik zerowy, głupi! ”); * P = 1;” nie daje ci ostrzeżenia.
gnasher729
2

Na przykład Objective-C traktuje każde wywołanie metody nilobiektu jako no-op, którego wartość jest zerowa. Istnieje kilka zalet tej decyzji projektowej w Objective-C, z powodów sugerowanych w twoim pytaniu. Teoretyczna koncepcja ochrony zerowej przed każdym wywołaniem metody ma swoje zalety, jeśli jest dobrze nagłośniona i konsekwentnie stosowana.

To powiedziawszy, kod żyje w ekosystemie, a nie próżni. Zachowane z zachowaniem zerowej ochrony byłoby w C ++ nie-idiomatyczne i zaskakujące , dlatego powinno być uważane za szkodliwe. Podsumowując, nikt nie pisze w ten sposób kodu C ++, więc nie rób tego! Jako kontrprzykład zauważ jednak, że wywołanie free()lub deletepołączenie NULLw C i C ++ jest gwarantowane, że nie działa.


W twoim przykładzie prawdopodobnie warto umieścić w konstruktorze twierdzenie, które msgSendernie ma wartości null. Jeśli konstruktor nazywa metodę na msgSenderod razu, to nie takie twierdzenie byłoby konieczne, gdyż to właśnie tam w każdym razie awarii. Ponieważ jednak jest on tylko przechowywany msgSender do wykorzystania w przyszłości, nie byłoby oczywiste, biorąc pod uwagę ślad stosu, SignalShutdown()jak powstała wartość NULL, więc zapewnienie w konstruktorze znacznie ułatwi debugowanie.

Co więcej, konstruktor powinien zaakceptować const IMsgSender&odwołanie, które nie może być NULL.

200_sukces
źródło
1

Powodem, dla którego jesteś proszony o unikanie zerowych dereferencji, jest upewnienie się, że kod jest niezawodny. Przykłady młodszych programistów dawno temu są tylko przykładami. Każdy może złamać kod przez przypadek i spowodować zerowe odwołanie - szczególnie w przypadku globali i globali klasowych. W C i C ++ jest to jeszcze bardziej możliwe przypadkowo, dzięki możliwości bezpośredniego zarządzania pamięcią. Możesz być zaskoczony, ale takie rzeczy zdarzają się bardzo często. Nawet przez bardzo kompetentnych, bardzo doświadczonych i bardzo starszych programistów.

Nie musisz zerować wszystkiego, ale musisz chronić się przed dereferencjami, które mają przyzwoite prawdopodobieństwo bycia zerowym. Dzieje się tak zwykle, gdy są one przydzielane, używane i usuwane z funkcji w różnych funkcjach. Możliwe, że jedna z pozostałych funkcji zostanie zmodyfikowana i zepsuje twoją funkcję. Możliwe jest również, że jedna z pozostałych funkcji może zostać wywołana nie w porządku (tak jak w przypadku dealokatora, który można wywołać oddzielnie od destruktora).

Wolę podejście, które mówią ci współpracownicy w połączeniu z używaniem asercji. Awaria w środowisku testowym, więc bardziej oczywiste jest, że istnieje problem do naprawienia i bezproblemowo podczas produkcji.

Powinieneś także używać niezawodnego narzędzia do poprawiania kodu, takiego jak coverity lub fortyfikacja. I powinieneś zająć się wszystkimi ostrzeżeniami kompilatora.

Edycja: jak wspomnieli inni, cicha awaria, jak w kilku przykładach kodu, jest również ogólnie rzeczą niewłaściwą. Jeśli twoja funkcja nie może odzyskać wartości zerowej, powinna zwrócić błąd (lub zgłosić wyjątek) do swojego obiektu wywołującego. Osoba dzwoniąca jest odpowiedzialna za ustalenie kolejności połączeń, odzyskanie lub zwrócenie błędu (lub zgłoszenie wyjątku) do osoby dzwoniącej i tak dalej. W końcu albo funkcja jest w stanie z wdziękiem odzyskać i przejść dalej, z wdziękiem odzyskać i zakończyć się niepowodzeniem (na przykład baza danych nie powiedzie się w transakcji z powodu błędu wewnętrznego dla jednego użytkownika, ale nie kończy się automatycznie), lub funkcja ustali, że stan aplikacji jest uszkodzony i nie można go odzyskać, a aplikacja kończy działanie.

atk
źródło
+1 za odwagę poparcia (pozornie) niepopularnej pozycji. Byłbym rozczarowany, gdyby nikt nie bronił moich kolegów w tej sprawie (są to bardzo mądrzy ludzie, którzy od wielu lat wypuszczają produkt wysokiej jakości). Podoba mi się również pomysł, że aplikacje powinny „ulec awarii w środowisku testowym ... i z wdziękiem zawieść w środowisku produkcyjnym”. Nie jestem przekonany, że mandat „rote” NULLto sposób, aby to zrobić, ale doceniam usłyszenie opinii kogoś spoza mojego miejsca pracy, który w zasadzie mówi: „Aha. Nie wydaje się to zbyt nierozsądne”.
evadeflow,
Denerwuje mnie, gdy widzę ludzi testujących NULL po malloc (). Mówi mi, że nie rozumieją, jak nowoczesne systemy operacyjne zarządzają pamięcią.
Kristof Provost
2
Domyślam się, że @KristofProvost mówi o systemach operacyjnych, które używają funkcji overcommit, dla których malloc zawsze się udaje (ale system operacyjny może później zabić proces, jeśli w rzeczywistości nie ma wystarczającej ilości pamięci). Nie jest to jednak dobry powód, aby pomijać kontrole zerowe w malloc: overcommit nie jest uniwersalny na różnych platformach, a nawet jeśli jest włączony, mogą istnieć inne przyczyny niepowodzenia malloc (np. W systemie Linux, limit przestrzeni adresowej procesu ustawiony za pomocą ulimit ).
John Bartłomiej
1
Co powiedział John. Rzeczywiście mówiłem o nadmiernym zaangażowaniu. Overcommit jest domyślnie włączony w Linuksie (co płaci moje rachunki). Nie wiem, ale podejrzewam, że tak samo jest w systemie Windows. W większości przypadków i tak się zawiesi, chyba że jesteś przygotowany napisać dużo kodu, który nigdy, nigdy nie zostanie przetestowany pod kątem obsługi błędów, które prawie na pewno nigdy się nie zdarzą ...
Kristof Provost
2
Dodam, że nigdy nie widziałem kodu (poza jądrem Linuksa, w którym brak pamięci jest realistycznie możliwy), który właściwie poradziłby sobie z brakiem pamięci. W każdym razie cały kod, który widziałem, po prostu się zawiesił. Wszystko, co udało się osiągnąć, to marnowanie czasu, utrudnienie zrozumienia kodu i ukrycie prawdziwego problemu. Dereferencje zerowe są łatwe do debugowania (jeśli masz plik podstawowy).
Kristof Provost
1

Użycie wskaźnika zamiast odwołania powiedziałoby mi, że msgSenderjest to tylko opcja i tutaj sprawdzanie wartości null byłoby prawidłowe. Fragment kodu jest zbyt wolny, aby na to zdecydować. Może są inne elementy, PowerManagerktóre są cenne (lub testowalne) ...

Wybierając pomiędzy wskaźnikiem a odniesieniem, dokładnie ważę obie opcje. Jeśli muszę użyć wskaźnika dla członka (nawet dla członków prywatnych), muszę zaakceptować tę if (x)procedurę za każdym razem, gdy go odłożę.

Wilk
źródło
Jak wspomniałem gdzieś w innym komentarzu, zdarza się, że wstrzyknięcie referencji nie jest możliwe. Przykładem, na który często
wpadam,
@evadeflow: OK, przyznaję, to zależy od wielkości klasy i widoczności członków wskaźnika (których nie można uczynić referencjami), czy sprawdzę przed dereferencją. W przypadkach, gdy klasa jest mała i prosta, a wskaźnik (wskaźniki) są prywatne, ja nie ...
Wolf
0

To zależy od tego, co chcesz się wydarzyć.

Napisałeś, że pracujesz nad „urządzeniem elektroniki użytkowej”. Jeśli jakiś błąd zostanie wprowadzony przez ustawienie msgSender_na NULL, chcesz

  • urządzenie do kontynuowania, pomijanie, SignalShutdownale kontynuowanie reszty działania, lub
  • urządzenie ulega awarii, zmuszając użytkownika do ponownego uruchomienia?

W zależności od wpływu, że sygnał wyłączenia nie jest wysyłany, opcja 1 może być dobrym wyborem. Jeśli użytkownik może nadal słuchać swojej muzyki, ale wyświetlacz nadal pokazuje tytuł poprzedniego utworu, może to być lepsze niż całkowite uszkodzenie urządzenia.

Oczywiście, jeśli wybierzesz opcję 1, assert(zalecane przez innych) jest niezbędne, aby zmniejszyć prawdopodobieństwo, że taki błąd wkradnie się niezauważony podczas programowania. Zabezpieczenie ifzerowe jest właśnie dostępne w celu ograniczenia awarii w zastosowaniach produkcyjnych.

Osobiście wolę też podejście „wczesne awarie” dla kompilacji produkcyjnych, ale rozwijam oprogramowanie biznesowe, które można łatwo naprawić i zaktualizować w przypadku błędu. W przypadku urządzeń elektroniki użytkowej może to nie być takie proste.

Heinzi
źródło