2012-01-10 1 views
11

miei recensori codice ha fatto notare che l'uso dell'operatore [] della mappa è molto male e portare a errori:stl map operator [] cattivo?

map[i] = new someClass; // potential dangling pointer when executed twice 

O

if (map[i]==NULL) ...  // implicitly create the entry i in the map 

Anche se capisco il rischio dopo aver letto il API che il insert() è meglio di quanto si verifica per duplicato, quindi può evitare che il puntatore penzolante accada, non capisco che se gestita correttamente, perché [] non può essere utilizzato affatto?

Seleziono la mappa come contenitore interno proprio perché desidero utilizzare la sua capacità di indicizzazione rapida e intuitiva.

Spero che qualcuno può o discutere di più con me o stare dalla mia parte :)

+5

In generale, le regole "mai fare questo" sono cattive. Ci sono casi in cui hai effettivamente bisogno di qualcosa e dovresti infrangere la regola. –

+1

(Su una nota correlata, vedere [Boost Pointer Containers] (http://www.boost.org/doc/libs/1_48_0/libs/ptr_container/doc/ptr_container.html) per i contenitori che manterranno i puntatori e li distruggeranno quando necessario) –

+0

Evitare di usare [] e preferire inserire/trovare. Questo è un buon senso, come evitare variabili non inizializzate, avvisi del compilatore ecc. – rmflow

risposta

10

L'unica volta (che ho dove può essere utile) dove operator[] può essere utile quando si desidera impostare il valore di una chiave (sovrascriverlo se ha già un valore) e si sa che è sicuro sovrascrivere (che dovrebbe essere poiché si dovrebbe utilizzare puntatori intelligenti, non puntatori grezzi) ed è economico per il valore predefinito e, in alcuni contesti, il valore dovrebbe avere no-throw construction and assignment.

ad es. (Simile al tuo primo esempio)

std::map<int, std::unique_ptr<int>> m; 
m[3] = std::unique_ptr<int>(new int(5)); 
m[3] = std::unique_ptr<int>(new int(3)); // No, it should be 3. 

caso contrario, ci sono alcuni modi per farlo a seconda del contesto, tuttavia mi sento di raccomandare di usare sempre la soluzione generale (in questo modo non si può sbagliare).

Trova un valore e crearlo se non esiste:

1. Soluzione Generale (consigliato in quanto funziona sempre)

std::map<int, std::unique_ptr<int>> m; 
auto it = m.lower_bound(3); 
if(it == std::end(m) || m.key_comp()(3, it->first)) 
    it = m.insert(it, std::make_pair(3, std::unique_ptr<int>(new int(3))); 

2. Con la costruzione di default a buon mercato di valore

std::map<int, std::unique_ptr<int>> m; 
auto& obj = m[3]; // value is default constructed if it doesn't exists. 
if(!obj) 
{ 
    try 
    { 
     obj = std::unique_ptr<int>(new int(3)); // default constructed value is overwritten. 
    } 
    catch(...) 
    { 
     m.erase(3); 
     throw; 
    } 
} 

3.Con la costruzione di default a buon mercato e l'inserimento tiro alcun valore

std::map<int, my_objecct> m; 
auto& obj = m[3]; // value is default constructed if it doesn't exists. 
if(!obj) 
    obj = my_objecct(3); 

Nota: Si potrebbe facilmente avvolgere la soluzione generale in un metodo di supporto:

template<typename T, typename F> 
typename T::iterator find_or_create(T& m, const typename T::key_type& key, const F& factory) 
{ 
    auto it = m.lower_bound(key); 
    if(it == std::end(m) || m.key_comp()(key, it->first)) 
     it = m.insert(it, std::make_pair(key, factory())); 
    return it; 
} 

int main() 
{ 
    std::map<int, std::unique_ptr<int>> m; 
    auto it = find_or_create(m, 3, [] 
    { 
     return std::unique_ptr<int>(new int(3)); 
    }); 
    return 0; 
} 

Nota che passo davanti a un metodo factory su modelli, invece di un valore per il caso di creazione, in questo modo non c'è un sovraccarico quando il valore è stato trovato e non è necessario creare. Poiché lambda viene passato come argomento modello, il compilatore può scegliere di inserirlo.

+0

Penso che hai perso una parola nella tua prima frase –

+0

@LightnessRacesinOrbit: Infatti. – ronag

+1

Evviva! Trovato: P –

0

Se la mappa è come per esempio questo:

std::map< int, int* > 

poi si perde, perché il codice seguente frammento sarebbe perdita memoria:

std::map< int, int* > m; 
m[3] = new int(5); 
m[3] = new int(2); 

se gestita correttamente, perché [] non può essere usato affatto?

Se il codice è stato testato correttamente, il codice deve ancora fallire la revisione del codice, poiché sono stati utilizzati puntatori non elaborati.

Altro quindi che, se usato correttamente, non c'è niente di sbagliato con l'utilizzo di map::operator[]. Tuttavia, sarebbe probabilmente meglio usare i metodi insert/find, a causa della possibile modifica silenziosa della mappa.

+3

Non c'è nulla di specifico per 'op []' nel tuo esempio lì. È come se avessi sovrascritto un puntatore. Questo non è un motivo per cui 'map''s' op [] 'è" cattivo ". –

1
  1. operator [] viene evitato per l'inserimento, perché per lo stesso motivo lei ha citato nella sua interrogazione. Non controlla la chiave duplicata e sovrascrive quella esistente.
  2. operator [] è in gran parte evitato per la ricerca nel std::map. Perché, se una chiave non esiste nel vostro map, poi operator [] sarebbe silenzio creare nuova chiave e inizializzare (tipicamente a 0). Che potrebbe non essere preferibile in tutti i casi. Si dovrebbe usare [] solo se è necessario creare una chiave, se non esiste.
+0

Allora la mia domanda sarà, quando dovremmo davvero usare l'operatore allora? L'indicizzazione non è forse la più grande caratteristica della mappatura in generale? – Figo

+0

Un'altra domanda: come posso verificare se una voce di una determinata indicizzazione è vuota, allora? E quando passiamo, se (map [i] == NULL) non è così male, se il secondo della coppia è un puntatore, verrà inizializzato a zero, giusto? – Figo

+0

@Figo, mentre si trova un elemento, se non si trova una chiave, restituisce 'end()' iteratore. Se nella propria applicazione, è necessario creare una chiave se non è stata trovata, quindi utilizzare 'operator []'. – iammilind

4

Hai ragione che map::operator[] deve essere usato con cautela, ma può essere molto utile: se si desidera trovare un elemento nella mappa, e se non ci crearlo:

someClass *&obj = map[x]; 
if (!obj) 
    obj = new someClass; 
obj->doThings(); 

E c'è solo una ricerca nella mappa. Se il new fallisce, si consiglia di rimuovere il puntatore NULL dalla mappa, ovviamente:

someClass *&obj = map[x]; 
if (!obj) 
    try 
    { 
     obj = new someClass; 
    } 
    catch (...) 
    { 
     obj.erase(x); 
     throw; 
    } 
obj->doThings(); 

Naturalmente, se si vuole trovare qualcosa, ma non per inserirla:

std::map<int, someClass*>::iterator it = map.find(x); //or ::const_iterator 
if (it != map.end()) 
{ 
    someClass *obj = it->second; 
    obj->doThings(); 
} 
+0

Penso che il try/catch mostri che è una cattiva idea ... potresti farlo più semplice con find/insert. – ronag

+0

Sì, ma find/insert cercherà l'elemento due volte. operator [] lo farà in un solo accesso! – rodrigo

+1

Abbastanza vero, se questo è un problema dovresti usare lower_bound/insert. Vedi la mia risposta aggiornata. – ronag

2

Affermazioni come "l'uso dell'operatore [] della mappa è pessimo" dovrebbe sempre essere un segnale di avvertimento di una credenza quasi religiosa. Ma come con la maggior parte di tali affermazioni, c'è un po 'di verità in agguato da qualche parte. La verità qui però è come con qualsiasi altro costrutto nella libreria standard C++: stai attento e sai cosa stai facendo. È possibile (accidentalmente) abusare di quasi tutto.

Un problema comune è potenziali perdite di memoria (assumendo che la vostra mappa possiede gli oggetti):

std::map<int,T*> m; 
m[3] = new T; 
... 
m[3] = new T; 

Ciò, ovviamente, perdita di memoria, in quanto sovrascrive il puntatore. Utilizzando inserto qui correttamente non è facile neanche, e molte persone fanno un errore che possa perdere in ogni modo, come:

std::map<int,T*> m; 
minsert(std::make_pair(3,new T)); 
... 
m.insert(std::make_pair(3,new T)); 

Anche se questo non cancella il vecchio puntatore, non inserirà il nuovo e anche perdite di esso. Il modo corretto con inserto sarebbe (forse meglio rafforzata con puntatori intelligenti):

std::map<int,T*> m; 
m.insert(std::make_pair(3,new T)); 
.... 
T* tmp = new T; 
if(!m.insert(std::make_pair(3,tmp))) 
{ 
    delete tmp; 
} 

Ma questo è un po 'troppo brutto. Io personalmente preferisco per questi casi semplici:

std::map<int,T*> m; 

T*& tp = m[3]; 
if(!tp) 
{ 
    tp = new T; 
} 

Ma questa è forse la stessa quantità di preferenze personali, come i revisori di codice hanno per non permettere l'utilizzo op [] ...

+0

Cosa succede se "nuova T" getta? Quindi hai una voce "vuota" nella mappa. – ronag

+0

@ronag: è per questo che sto parlando di puntatori intelligenti. Questo non è inteso come un esempio perfetto, ma piuttosto per trasferire l'idea, in quanto deve essere comunque adattata alla situazione. – PlasmaHH

+0

Ti incoraggerei comunque a scrivere un esempio "corretto". In che modo l'utilizzo di un puntatore intelligente cambia il fatto che si avrà una voce vuota nella mappa se getta "new". – ronag

0
map[i] = new someClass; // potential dangling pointer when executed twice 

qui, il problema non è di carta operator[], ma * la mancanza di puntatori intelligenti. Il puntatore deve essere memorizzato in un oggetto RAII (come un puntatore intelligente), che acquisisce immediatamente la proprietà dell'oggetto assegnato e garantisce che venga liberato.

Se i revisori del codice ignorano questo, e invece affermano che dovresti provare avidamente lo operator[], acquistale un buon libro di testo C++.

if (map[i]==NULL) ...  // implicitly create the entry i in the map 

È vero. Ma questo perché operator[] è progettato per comportarsi in modo diverso. Ovviamente, non dovresti usarlo in situazioni in cui fa la cosa sbagliata.

+0

Non vedo dove puntatori intelligenti avrebbero risolto e il problema generale (che in questo caso potrebbe essere l'uso di puntatori all'inizio, quando la semantica del valore è più appropriata). Ci sono alcuni casi in cui i puntatori intelligenti sono appropriati, ma devo ancora vedere un caso in cui una mappa di puntatori intelligenti era la soluzione corretta. –

+0

@JamesKanze: ma risolverebbero il problema specifico che "se chiamo' new' e memorizzo il risultato nello stesso oggetto di destinazione, riuscirò a perdere memoria ", che sembra essere il presunto problema nel primo caso e che ha molto poco a che fare con 'std :: map'. Ho frainteso l'OP? – jalf

+0

Il problema specifico in questo caso non è probabilmente l'uso di 'map []', ma l'uso di 'new'. Se si utilizza la semantica del valore, piuttosto che 'new' e un puntatore, non si ha una perdita, indipendentemente da ciò che accade. (Un altro possibile problema specifico è che il codice non prevede di sostituire un valore esistente. In questo caso, ovviamente, si deve usare 'insert', piuttosto che' [] ', con un test del valore restituito.) –

0

Generalmente il problema è che l'operatore [] crea implicitamente un valore associato alla chiave passata e inserisce una nuova coppia nella mappa se la chiave non si verifica già. Ciò può interrompere la logica da quel momento in poi, ad es. quando cerchi se esiste una determinata chiave.

map<int, int> m; 
if (m[4] != 0) { 
    cout << "The object exists" << endl; //furthermore this is not even correct 0 is totally valid value 
} else { 
    cout << "The object does not exist" << endl; 
} 
if (m.find(4) != m.end()) { 
    cout << "The object exists" << endl; // We always happen to be in this case because m[4] creates the element 
} 

mi consiglia di utilizzare l'operatore [] solo quando si sa sarete indicare una chiave già esistente nella mappa (questo tra l'altro dimostra di essere non caso in modo non frequente).

+0

Ma come puoi essere sicuro che la chiave non sia già presente? L'unico uso corretto di 'operator []' lo so è quando vuoi un nuovo oggetto costruito di default se l'oggetto non è già presente (ad esempio cose come le mappe dei simboli, dove ogni simbolo che vedi nella sorgente dovrebbe essere presente nella mappa). –

+0

Non si incontra se la chiave è presente: viene restituito il valore esistente. Il problema si verifica quando non esiste: viene creato un nuovo oggetto.Esempio di un caso in cui sai che un oggetto è presente è quando le tue chiavi provengono da una sorta di enumerazione. –

+0

Quando si utilizza un'enumerazione come tipo di chiave di 'std :: map'? Se il tipo di chiave è un'enumerazione, userei un 'std :: vector', o anche un array in stile C. Non conosco alcun caso in cui potrei essere sicuro che la chiave fosse già presente, per tutta la durata del codice. (Anche con enum, qualcuno potrebbe aggiungere un valore all'enumerazione, ma dimentica di modificare l'inizializzazione della mappa per includerla.) –

1

Questo non è un problema con [] affatto. È un problema con la memorizzazione dei puntatori grezzi nei contenitori.

+0

Non è un problema con i puntatori grezzi nei contenitori di per sé. È (probabilmente) un problema nell'utilizzo di puntatori quando la semantica del valore è più appropriata. –

+0

@James: non sono d'accordo. Un "valore_tipo" del puntatore condiviso può avere senso qui. –

+0

Altamente improbabile. Circa l'unico caso a cui riesco a pensare è se l'oggetto è polimorfico. E anche allora: nei casi che ho visto, gli oggetti hanno avuto una durata statica, o sono stati inseriti nel costruttore e rimossi nel distruttore; in entrambi i casi, un 'shared_ptr' causerà problemi, non li risolverà. –

0

Non c'è niente di sbagliato in operator[] di mappa, di per sé, finché la sua semantica corrisponde a ciò che si desidera. Il problema è definire ciò che vuoi (e conoscere la semantica esatta di operator[]). Ci sono momenti in cui creativamente una nuova voce con un valore predefinito quando la voce non è presente è esattamente ciò che si desidera (ad esempio contando le parole in un documento di testo , dove ++ countMap[word] è tutto ciò che serve); ci sono molte altre volte che non lo è.

Un problema più serio nel codice potrebbe essere quello di memorizzare i puntatori nella mappa. Una soluzione più naturale potrebbe essere quella di utilizzare un map <keyType, someClass> anziché un map <keyType, SomeClass*>. Ma ancora una volta, questo dipende dalla semantica desiderata; ad esempio, utilizzo un gran numero di map che vengono inizializzate una volta all'avvio del programma con puntatori alle istanze statiche . Se sei map[i] = ... in un ciclo di inizializzazione, eseguito una volta all'avvio, probabilmente non ci sono problemi. Se si tratta di qualcosa di eseguito in molti punti diversi del codice, probabilmente c'è un problema .

La soluzione al problema non è vietare operator[] (o mappe per i puntatori ). La soluzione è iniziare specificando la semantica esatta di cui hai bisogno. E se std::map non li fornisce direttamente (raramente lo fa ), scrivi una piccola classe wrapper che definisce la semantica esatta che desideri , utilizzando std::map per implementarli. Così, il vostro wrapper per operator[] potrebbe essere:

MappedType MyMap::operator[](KeyType const& key) const 
{ 
    MyMap::Impl::const_iterator elem = myImpl.find(key); 
    if (elem == myImpl.end()) 
     throw EntryNotFoundError(); 
    return elem->second; 
} 

o:

MappedType* MyMap::operator[](KeyType const& key) const 
{ 
    MyMap::Impl::const_iterator elem = myImpl.find(key); 
    return elem == myImpl.end() 
     ? NULL // or the address of some default value 
     : &elem->second; 
} 

Allo stesso modo, si potrebbe desiderare di utilizzare insert piuttosto che operator[] se davvero si vuole inserire un valore che non è già presente.

E non ho quasi mai visto un caso in cui si inserisce immediatamente un oggetto new in una mappa. La ragione di consueto per l'utilizzo di new e delete è che gli oggetti in questione hanno una certa durata specifica di propria (e non sono copiabili — anche se non è una regola assoluta, se sei new ing un oggetto che supporta la copia e l'assegnazione, sei probabilmente facendo qualcosa di sbagliato). Quando il tipo mappato è un puntatore, quindi gli oggetti puntati agli oggetti sono statici (e la mappa è più o meno costante dopo l'inizializzazione), oppure l'inserimento e la rimozione sono eseguiti nel costruttore e nel distruttore della classe. (Ma questa è solo la una regola generale, ci sono certamente delle eccezioni.)