STL оператор карты [] плохо? - PullRequest
       13

STL оператор карты [] плохо?

11 голосов
/ 10 января 2012

Мои обозреватели кода отмечают, что использование оператора [] карты очень плохо и приводит к ошибкам:

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

Или

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

Хотя после прочтения API я понимаю риск того, что insert() лучше, так как он проверяет наличие дубликатов, что позволяет избежать появления висячих указателей, но я не понимаю, что при правильной обработке [] может не будет использоваться вообще?

Я выбираю карту в качестве своего внутреннего контейнера именно потому, что хочу использовать ее быструю и понятную возможность индексации.

Надеюсь, кто-то может больше спорить со мной или встать на мою сторону :)

Ответы [ 9 ]

11 голосов
/ 10 января 2012

Единственное время (о котором я могу подумать), где operator[] может быть полезным, это когда вы хотите установить значение ключа (перезаписать его, если оно уже имеет значение), и вы знаете, что это безопасно перезаписать (что и должно быть, так как вы должны использовать умные указатели, а не необработанные указатели) и дешевле для конструкции по умолчанию , а в некоторых контекстах значение должно иметь no-бросить конструкцию и присваивание .

например (похоже на ваш первый пример)

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.

В противном случае есть несколько способов сделать это в зависимости от контекста, однако я бы рекомендовал всегдаиспользуйте общее решение (таким образом, вы не сможете ошибиться).

Найдите значение и создайте его, если оно не существует:

1.Общее решение (рекомендуется, поскольку оно всегда работает)

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.С дешевой конструкцией по умолчанию значения

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.С дешевой конструкцией по умолчанию и вставкой без бросков значения

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

Примечание: вы можете легко обернуть общее решение в вспомогательный метод:

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;
}

Обратите внимание, что я передаюшаблонный фабричный метод вместо значения для случая создания, таким образом нет никаких накладных расходов, когда значение было найдено и его не нужно создавать.Поскольку лямбда-выражение передается в качестве аргумента шаблона, компилятор может включить его.

6 голосов
/ 10 января 2012

Вы правы, что map::operator[] нужно использовать с осторожностью, но это может быть весьма полезно: если вы хотите найти элемент на карте, а если нет, создайте его:

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

И на карте есть только один поиск.В случае сбоя new вы можете удалить указатель NULL с карты, конечно:

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

Естественно, если вы хотите что-то найти, но не вставлять:

std::map<int, someClass*>::iterator it = map.find(x); //or ::const_iterator
if (it != map.end())
{
    someClass *obj = it->second;
    obj->doThings();
}
2 голосов
/ 10 января 2012

Утверждения типа «использование оператора [] карты очень плохо» всегда должны быть предупреждением о почти религиозных убеждениях.Но, как и в большинстве подобных претензий, где-то скрывается доля правды.Правда здесь, как и у любой другой конструкции в стандартной библиотеке C ++: будьте осторожны и знайте, что вы делаете.Вы можете (случайно) злоупотреблять почти всем.

Одной из распространенных проблем являются потенциальные утечки памяти (при условии, что вашей карте принадлежат объекты):

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

Это, очевидно, приведет к утечке памяти, поскольку перезаписывает указатель.Правильно использовать вставку здесь также непросто, и многие люди допускают ошибку, которая в любом случае будет иметь утечку, например:

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

Хотя это не перезапишет старый указатель, он не вставит новый, а также утечкуЭто.Правильный способ вставки будет (возможно, лучше улучшен умными указателями):

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;
}

Но это тоже несколько уродливо.Лично я предпочитаю для таких простых случаев:

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

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

Но, возможно, это то же самое количество личных предпочтений, что и у ваших рецензентов кода для запрета использования op [] ...

1 голос
/ 10 января 2012

Это вообще не проблема с [].Проблема с хранением сырых указателей в контейнерах.

1 голос
/ 10 января 2012
  1. operator [] избегается для вставки, потому что по той же причине Вы упомянули в своем вопросе. Не проверяет дубликат ключа и перезаписывает на существующий.
  2. operator [] в основном избегается при поиске в std::map. Потому что, если ключ не существует в вашем map, то operator [] тихо создаст новый ключ и инициализирует его (обычно 0). Что не может быть предпочтительным во всех случаях. Надо использовать [] только если необходимо создать ключ, если он не существует.
0 голосов
/ 10 января 2012

Нет ничего плохого в operator[] карты как таковой, если ее семантика соответствует тому, что вы хотите. Проблема заключается в определении того, что вы хочу (и зная точную семантику operator[]). Есть моменты когда неявно создается новая запись со значением по умолчанию, когда запись нет именно то, что вы хотите (например, подсчет слов в тексте документ, где ++ countMap[word] - это все, что вам нужно); имеются во многих других случаях это не так.

Более серьезная проблема в вашем коде может заключаться в том, что вы храните указатели на карте. Более естественным решением может быть использование map <keyType, someClass>, а не map <keyType, SomeClass*>. Но опять же, это зависит от желаемой семантики; например, я использую много map которые инициализируются один раз, при запуске программы, с указателями на статические экземпляров. Если вы map[i] = ... в цикле инициализации, выполняется один раз при запуске, вероятно, нет проблем. Если это что-то выполняется в разных местах кода, вероятно, есть выпуск.

Решение проблемы не в том, чтобы запретить operator[] (или отображать на указатели). Решение состоит в том, чтобы начать с указания точной семантики тебе нужно. И если std::map не предоставляет их напрямую (это редко делает), напишите небольшой класс-обертку, который определяет точную семантику, которую вы хочу, используя std::map для их реализации. Таким образом, ваша обертка для operator[] может быть:

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

или

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;
}

Точно так же вы можете использовать insert вместо operator[], если Вы действительно хотите вставить значение, которого еще нет.

И я почти никогда не видел случая, чтобы вы сразу вставили new ред объекта на карту. Обычная причина использования new и delete в том, что рассматриваемые объекты имеют некоторое конкретное время жизни свои собственные (и не копируемые - хотя и не абсолютное правило, если вы new используете объект, который поддерживает копирование и присваивание, вы наверное что то не так делаю). Когда сопоставленный тип является указателем, тогда либо указанные объекты статичны (а карта более или менее постоянный после инициализации), или вставка и удаление сделано в конструкторе и деструкторе класса. (Но это просто общее правило; Есть, конечно, исключения.)

0 голосов
/ 10 января 2012

Обычно проблема заключается в том, что operator [] неявно создает значение, связанное с переданным ключом, и вставляет новую пару в карту, если ключ еще не существует.Это может нарушить вашу логику с тех пор, например, при поиске, существует ли определенный ключ.

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
}

Я рекомендую использовать оператор [] только тогда, когда вы знаете, что будете ссылаться на ключ, уже существующий на карте(это, кстати, бывает не так уж и редко).

0 голосов
/ 10 января 2012
 map[i] = new someClass;    // potential dangling pointer when executed twice

здесь проблема не в карте operator[], а в * отсутствии умных указателей. Ваш указатель должен быть сохранен в некотором объекте RAII (таком как умный указатель), который сразу же становится владельцем выделенного объекта и гарантирует его освобождение.

Если ваши обозреватели кода игнорируют это и вместо этого говорят, что вам следует усердно operator[], купите им хороший учебник по С ++.

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

Это правда. Но это потому, что operator[] разработан, чтобы вести себя по-другому. Очевидно, что вы не должны использовать его в ситуациях, когда он делает неправильные вещи.

0 голосов
/ 10 января 2012

Если ваша карта, например, такая:

std::map< int, int* >

тогда вы проиграете, потому что следующий фрагмент кода приведет к утечке памяти:

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

если обрабатывается правильно, почему [] вообще нельзя использовать?

Если вы правильно проверили свой код, то ваш код все равно должен не пройти проверку кода, потому что вы использовали необработанные указатели.

Кроме того, при правильном использовании, нет ничего плохого в использовании map::operator[]. Однако вам, вероятно, будет лучше использовать методы вставки / поиска из-за возможного изменения карты без вывода сообщений.

...