stl map operator [] cattivo?

Question

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

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 :)

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 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(); 
} 

Answer 5

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 [] ...

Answer 6

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.

Answer 7

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).

Answer 8

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

Answer 9

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.)