E 'ragionevole proteggere ogni singolo puntatore senza referenza?

Dipende dal "contratto":

Se PowerManager DEVE avere un IMsgSender valido, non verificare mai nulla, lasciarlo morire prima.

Se d'altra parte, MAGGIO ha un IMsgSender , quindi devi controllarlo ogni volta che lo usi, così semplice.

Commento finale sulla storia del programmatore junior, il problema è in realtà la mancanza di procedure di test.

Sento che il codice dovrebbe leggere:

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

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

Questo è in realtà meglio di proteggere il NULL, perché rende molto chiaro che la funzione non dovrebbe mai essere chiamata se msgSender_ è NULL. Si assicura anche che noterai se ciò accade.

La "saggezza" condivisa dei tuoi colleghi ignorerà silenziosamente questo errore, con risultati imprevedibili.

In generale i bug sono più facili da risolvere se vengono rilevati più vicini alla loro causa. In questo esempio la protezione NULL proposta porterebbe a un messaggio di arresto che non viene impostato, il che può provocare o meno un errore evidente. Avrai più difficoltà a tornare indietro alla funzione SignalShutdown rispetto a quando l'intera applicazione è appena morta, producendo un backtrace o dump core a portata di mano che punta direttamente a SignalShutdown() .

È un po 'poco intuitivo, ma il crash non appena qualcosa è sbagliato tende a rendere il tuo codice più robusto. Questo perché in realtà trova i problemi e tende ad avere anche cause molto ovvie.

Se msgSender non deve mai essere null , dovresti mettere il controllo null solo nel costruttore. Questo vale anche per tutti gli altri input nella classe - metti il controllo di integrità al punto di entrata nel 'modulo' - classe, funzione ecc.

La mia regola empirica è di eseguire controlli di integrità tra i limiti del modulo - la classe in questo caso. Inoltre, una classe dovrebbe essere abbastanza piccola da consentire di verificare rapidamente mentalmente l'integrità della vita dei membri della classe, assicurando che tali errori vengano eliminati in modo improprio / cancellazioni nulle. Il controllo nullo che viene eseguito nel codice nel tuo post presuppone che qualsiasi utilizzo non valido assegni effettivamente null al puntatore, il che non è sempre il caso. Poiché "utilizzo non valido" implica implicitamente che non si applicano ipotesi sul codice normale, non possiamo essere sicuri di rilevare tutti i tipi di errori del puntatore, ad esempio un'eliminazione non valida, un incremento ecc.

Inoltre, se sei certo che l'argomento non può mai essere nullo, considera l'utilizzo di riferimenti, a seconda dell'utilizzo della classe. Altrimenti, considera l'utilizzo di std::unique_ptr o std::shared_ptr al posto di un puntatore raw.

No, non è ragionevole controllare ogni e ogni dereferenziamento del puntatore per il puntatore che è NULL .

I controlli dei puntatori nulli sono preziosi sugli argomenti delle funzioni (inclusi gli argomenti del costruttore) per garantire che le precondizioni siano soddisfatte o per intraprendere l'azione appropriata se non viene fornito un parametro facoltativo e sono utili per verificare l'invarianza di una classe dopo aver esposto la classe interni. Ma se l'unica ragione per cui un puntatore è diventato NULL è l'esistenza di un bug, allora non ha senso controllare. Quel bug avrebbe potuto facilmente impostare il puntatore su un altro valore non valido.

Se mi trovassi di fronte a una situazione come la tua, vorrei porre due domande:

• Perché l'errore di quel programmatore junior non è stato catturato prima del rilascio? Ad esempio in una revisione del codice o nella fase di test? Portare sul mercato un dispositivo di elettronica di consumo difettoso può essere altrettanto costoso (se si tiene conto della quota di mercato e della benevolenza) come il rilascio di un dispositivo difettoso utilizzato in un'applicazione safety-critical, quindi mi aspetto che l'azienda sia seriamente testata e altre attività di controllo qualità.
• Se il controllo nullo fallisce, quale gestione degli errori si aspetta che io metta in atto, o potrei semplicemente scrivere assert(msgSender_) invece del controllo null? Se si inserisce il check-in null, è possibile che si sia verificato un arresto anomalo, ma è possibile che si sia creata una situazione peggiore in quanto il software continua sulla premessa che un'operazione è stata eseguita mentre in realtà l'operazione è stata saltata. Ciò potrebbe comportare l'instabilità di altre parti del software.

Questo esempio sembra essere più sulla durata dell'oggetto che sul fatto che un parametro di input sia nullo †. Dato che tu dici che PowerManager deve sempre avere un IMsgSender valido, passare l'argomento per puntatore (permettendo così la possibilità di un puntatore nullo) mi sembra un difetto di progettazione ††.

In situazioni come questa, preferirei cambiare l'interfaccia in modo che i requisiti del chiamante siano applicati dalla lingua:

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

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

Riscrivendolo in questo modo, afferma che PowerManager deve contenere un riferimento a IMsgSender per l'intera durata. Questo, a sua volta, stabilisce anche un requisito implicito che IMsgSender deve vivere più a lungo di PowerManager , e nega la necessità di qualsiasi controllo o asserzione di puntatore nullo all'interno di PowerManager .

Puoi anche scrivere la stessa cosa usando un puntatore intelligente (tramite boost o c ++ 11), per forzare esplicitamente IMsgSender a vivere più a lungo di 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()");
}

Questo metodo è preferibile se è possibile che la durata di IMsgSender non possa essere più lunga di PowerManager (cioè x = new IMsgSender(); p = new PowerManager(*x); ).

† Riguardo ai puntatori: il controllo nullo rampante rende il codice più difficile da leggere e non migliora la stabilità (migliora l'aspetto della stabilità, che è molto peggio).

Da qualche parte, qualcuno ha ricevuto un indirizzo per la memoria per contenere il IMsgSender . È responsabilità della funzione assicurarsi che l'allocazione abbia avuto successo (controllare i valori di ritorno della libreria o gestire correttamente std::bad_alloc eccezioni), in modo da non aggirare i puntatori non validi.

Poiché PowerManager non possiede il IMsgSender (lo prende in prestito solo per un po '), non è responsabile dell'allocazione o della distruzione di quella memoria. Questo è un altro motivo per cui preferisco il riferimento.

†† Dato che sei nuovo in questo lavoro, mi aspetto che tu stia hackerando il codice esistente. Quindi, per difetto di progettazione, intendo che il difetto è nel codice con cui stai lavorando. Pertanto, le persone che contrassegnano il tuo codice perché non sta controllando i puntatori nulli si stanno davvero segnalando per scrivere codice che richiede puntatori:)

Come le eccezioni, le condizioni di guardia sono utili solo se sai cosa fare per recuperare dall'errore o se vuoi dare un messaggio di eccezione più significativo.

Ingoiare un errore (che sia stato rilevato come eccezione o come controllo di guardia), è solo la cosa giusta da fare quando l'errore non ha importanza. Il posto più comune per me per vedere gli errori che vengono ingeriti è il codice di registrazione degli errori: non si vuole bloccare un'app perché non si è riusciti a registrare un messaggio di stato.

Ogni volta che viene chiamata una funzione, e non è un comportamento opzionale, dovrebbe fallire rumorosamente, non in modo silenzioso.

Modifica: sul modo di pensare alla storia del tuo programmatore junior, sembra che quello che è successo sia stato che un membro privato era stato impostato su null quando non si prevedeva che potesse accadere. Hanno avuto un problema con la scrittura inappropriata e stanno tentando di risolverlo convalidando la lettura. Questo è indietro. Nel momento in cui lo identificate, l'errore è già avvenuto. La soluzione applicabile al code-review / compilatore per questo non è condizioni di guardia, ma invece getter e setter o membri const.

Come altri hanno notato, ciò dipende dal fatto che msgSender possa essere legittimamente NULL . Quanto segue presuppone che dovrebbe mai essere NULL.

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

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

La "correzione" proposta dagli altri membri del tuo team viola il principio Dead Programs Tell No Lies . I bug sono davvero difficili da trovare così com'è. Un metodo che modifica in modo silenzioso il suo comportamento in base a un problema precedente, non solo rende difficile trovare il primo bug ma aggiunge anche un secondo bug.

Il junior ha causato il caos non controllando il null. Cosa succederebbe se questo pezzo di codice causasse il caos continuando a girare in uno stato indefinito (il dispositivo è acceso ma il programma "pensa" che sia spento)? Forse un'altra parte del programma farà qualcosa che è sicuro solo quando il dispositivo è spento.

Ciascuno di questi approcci eviterà errori silenziosi:

1. Usa gli asser come suggerito da questa risposta , ma assicurati che siano attivati nel codice di produzione . Questo, naturalmente, potrebbe causare problemi se altri asserzioni fossero state scritte con l'assunto che sarebbero state disattivate in produzione.

2. Genera un'eccezione se è nullo.

Sono d'accordo con il trapping del null nel costruttore. Inoltre, se il membro è dichiarato nell'intestazione come:

IMsgSender* const msgSender_;

Quindi il puntatore non può essere cambiato dopo l'inizializzazione, quindi se è stato eseguito correttamente, andrà bene per la durata dell'oggetto che lo contiene. (L'oggetto indicato da a sarà not essere const.)

Questo è definitivo pericoloso!

Ho lavorato con uno sviluppatore senior in un codeb C con gli "standard" più scadenti che hanno spinto per la stessa cosa, per controllare ciecamente tutti i puntatori di null. Lo sviluppatore finirebbe per fare cose del genere:

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

Una volta ho provato a rimuovere un tale controllo della precondizione una volta in tale funzione e a sostituirlo con una assert per vedere cosa sarebbe successo.

Con mio orrore, ho trovato migliaia di righe di codice nella base di codice che passavano null a questa funzione, ma dove gli sviluppatori, probabilmente confusi, hanno lavorato attorno e hanno aggiunto altro codice finché le cose non hanno funzionato.

Per il mio ulteriore orrore, ho trovato che questo problema era prevalente in tutti i tipi di posti nella base di codice che verificava i null. Il codebase è cresciuto nel corso dei decenni per fare affidamento su questi controlli al fine di poter violare in silenzio anche le precondizioni più esplicitamente documentate. Rimuovendo questi controlli mortali a favore di asserts , tutti gli errori umani logici per decenni nella base di codici verrebbero rivelati e noi li annegheremmo.

Ci sono volute solo due righe di codice apparentemente innocenti come questa + tempo e una squadra per finire con il mascherare un migliaio di bug accumulati.

Questi sono i tipi di pratiche che fanno sì che i bug dipendano da altri bug affinché il software funzioni. È uno scenario da incubo . Inoltre, ogni errore logico relativo alla violazione di tali precondizioni mostra misteriosamente un milione di righe di codice lontano dal sito reale in cui si è verificato l'errore, dal momento che tutti questi controlli nulli nascondono il bug e nascondono il bug finché non raggiungiamo un luogo che ha dimenticato per nascondere il bug.

Controllare semplicemente i valori nulli ciecamente in ogni luogo in cui un puntatore nullo viola una precondizione è, per me, pazzia totale, a meno che il tuo software sia così critico nei confronti di errori di asserzione e arresti di produzione che il potenziale di questo scenario sia preferibile.

Is it reasonable for a coding standard to require that every single pointer dereferenced in a function be checked for NULL first—even private data members?

Quindi direi, assolutamente no. Non è nemmeno "sicuro". Potrebbe benissimo essere il contrario e mascherare tutti i tipi di bug in tutta la base di codice che, nel corso degli anni, possono portare agli scenari più orribili.

assert è la strada da percorrere qui. Le violazioni delle precondizioni non dovrebbero essere lasciate inosservate, altrimenti la legge di Murphy può facilmente dare il calcio.

Objective-C , ad esempio, considera ogni chiamata di metodo su un oggetto nil come no-op che valuta di un valore zero-ish. Ci sono alcuni vantaggi in merito alla decisione di progettazione in Objective-C, per i motivi suggeriti nella tua domanda. Il concetto teorico di null-guardia di ogni chiamata di metodo ha qualche merito se è ben pubblicizzato e applicato in modo coerente.

Detto questo, il codice vive in un ecosistema, non in un vuoto. Il comportamento con protezione nulla sarebbe non-idiomatico e sorprendente in C ++ e pertanto dovrebbe essere considerato dannoso. In sintesi, nessuno scrive il codice C ++ in questo modo, quindi non fallo! Come controesempio, tuttavia, tieni presente che chiamare free() o delete su NULL in C e C ++ è garantito come non operativo.

Nel tuo esempio, sarebbe probabilmente utile inserire un'asserzione nel costruttore che msgSender non sia nullo. Se il costruttore ha chiamato subito un metodo su msgSender , allora nessuna asserzione di questo tipo sarebbe necessaria, dato che si arresterebbe comunque in ogni caso. Tuttavia, poiché è solo memorizzare msgSender per un uso futuro, non sarebbe ovvio osservare una traccia stack di SignalShutdown() come il valore è diventato NULL , quindi un'asserzione in il costruttore renderebbe il debugging significativamente più semplice.

Ancora meglio, il costruttore dovrebbe accettare un riferimento const IMsgSender& , che non può essere probabilmente NULL .

Il motivo per cui ti viene chiesto di evitare i dereferenze nulle è assicurarsi che il tuo codice sia solido. Esempi di programmatori junior molto tempo fa sono solo esempi. Chiunque può violare il codice per sbaglio e causare un dereferenziamento nullo, in particolare per i globali e le classi globali. In C e C ++ è ancora più possibile accidentalmente, con la capacità di gestione diretta della memoria. Potresti essere sorpreso, ma questo genere di cose accade molto spesso. Anche da sviluppatori molto esperti, molto esperti e molto anziani.

Non hai bisogno di nulla controllare tutto, ma devi proteggerti contro i dereferenze che hanno una buona probabilità di essere nulli. Questo è comunemente quando sono assegnati, usati e dereferenziati in diverse funzioni. È possibile che una delle altre funzioni venga modificata e interrompa la funzione. È anche possibile che una delle altre funzioni possa essere chiamata fuori servizio (come se si disponga di un deallocator che può essere chiamato separatamente dal distruttore).

Preferisco l'approccio che i tuoi colleghi ti stanno dicendo in combinazione con l'uso di assert. Crash nell'ambiente di test, quindi è più ovvio che c'è un problema da risolvere e fallire con garbo nella produzione.

Dovresti anche usare un robusto strumento di correttezza del codice come coverity o fortify. E dovresti affrontare tutti gli avvertimenti del compilatore.

Modifica: come altri hanno già accennato, il fallimento silenzioso, come in molti degli esempi di codice, è generalmente anche la cosa sbagliata. Se la tua funzione non può recuperare dal valore che è null, dovrebbe restituire un errore (o lanciare un'eccezione) al suo chiamante. Il chiamante è responsabile della risoluzione del proprio ordine di chiamata, del ripristino o della restituzione di un errore (o di un'eccezione) al chiamante e così via. Alla fine, una funzione è in grado di ripristinare e andare avanti in modo garbato, ripristinarsi con garbo e fallire (come un database che non riesce a effettuare una transazione a causa di un errore interno per un utente ma non in uscita) oppure la funzione determina che lo stato dell'applicazione è corrotto e irrecuperabile e l'applicazione termina.

L'uso di un puntatore invece di un riferimento mi direbbe che msgSender è solo un'opzione e qui il controllo Null sarebbe avere ragione. Lo snippet di codice è troppo lento per decidere che. Forse ci sono altri elementi in PowerManager che sono preziosi (o testabili) ...

Quando si sceglie tra il puntatore e il riferimento, peserò a fondo entrambe le opzioni. Se devo usare un puntatore per un membro (anche per i membri privati), devo accettare la procedura if (x) ogni volta che la dereferenziamento.

Dipende da ciò che vuoi che accada.

Hai scritto che stai lavorando su "un dispositivo elettronico di consumo". Se, in qualche modo, viene introdotto un bug impostando msgSender_ su NULL , vuoi

• il dispositivo da portare avanti, saltando SignalShutdown ma continuando il resto della sua operazione, o
• il dispositivo si blocca, costringendo l'utente a riavviarlo?

A seconda dell'impatto che il segnale di spegnimento non viene inviato, l'opzione 1 potrebbe essere una scelta praticabile. Se l'utente può continuare ad ascoltare la sua musica, ma il display mostra ancora il titolo della traccia precedente, potrebbe essere preferibile a un arresto completo del dispositivo.

Naturalmente, se scegli l'opzione 1, un assert (come raccomandato da altri) è vitale per ridurre la probabilità che un tale bug si insinui inosservato durante lo sviluppo. Il if null guard è solo lì per la mitigazione dei guasti nell'uso della produzione.

Personalmente, preferisco anche l'approccio "crash early" per le build di produzione, ma sto sviluppando un software aziendale che può essere risolto e aggiornato facilmente in caso di bug. Per i dispositivi elettronici di consumo, questo potrebbe non essere così facile.