c++ 为什么std::map::operator[]被认为是不好的做法?

roejwanj  于 11个月前  发布在  其他
关注(0)|答案(9)|浏览(151)

我的代码评审员指出,map的operator[]的使用非常糟糕,会导致错误:

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

字符串

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


虽然我在阅读了API后明白了insert()更好的风险,因为它检查了重复,从而可以避免悬空指针的发生,但我不明白,如果处理得当,为什么[]根本不能使用?
我选择map作为我的内部容器,正是因为我想使用它的快速和自我解释的索引功能。
我希望有人能和我多争论一下,或者站在我这边:)

xcitsw88

xcitsw881#

唯一一次(我能想到的)当你想设置一个键的值时,operator[]会很有用。(如果它已经有值,则覆盖它),并且您知道覆盖是安全的(这应该是因为你应该使用智能指针,而不是原始指针),并且默认构造便宜,在某些情况下,值应该有no-throw构造和赋值
例如(类似于第一个例子)

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


注意:你可以很容易地将通用解决方案 Package 到一个helper方法中:

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


注意,我传递了一个模板化的工厂方法,而不是一个值,这样当值被发现时就没有开销了,也不需要创建。因为lambda是作为模板参数传递的,编译器可以选择内联它。

hm2xizp9

hm2xizp92#

map::operator[]必须小心使用,这是正确的,但它可能非常有用:如果你想在map中找到一个元素,如果没有,就创建它:

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

字符串
在map中只有一个查找。如果new失败,你可能想从map中删除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();
}

ycl3bljg

ycl3bljg3#

像“使用map的operator[]是非常糟糕的”这样的声明应该永远是一个警告信号,几乎是宗教信仰。但是就像大多数这样的声明一样,有一点真理潜伏在某处。然而,这里的真理就像C++标准库中的几乎任何其他构造一样:小心并知道你在做什么。你可以(意外地)误用几乎所有东西。
一个常见的问题是潜在的内存泄漏(假设你的map拥有这些对象):

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

字符串
这显然会泄漏内存,因为它会覆盖指针。在这里正确使用insert也不容易,许多人会犯一个错误,无论如何都会泄漏,比如:

std::map<int,T*> m;
m.insert(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;
}


但这可能与您的代码评审员不允许使用operator[]的个人偏好相同。

ua4mk5z4

ua4mk5z44#

  1. operator []是避免插入,因为出于同样的原因,你在你的问题中提到.它不检查重复的关键和覆盖现有的.
  2. operator []std::map中搜索时通常避免使用。因为,如果map中不存在某个键,那么operator []静默地创建新的键并初始化它(通常为0)。这可能不是在所有情况下都是首选。只有在需要创建键时才应该使用[],如果它不存在。
vwoqyblh

vwoqyblh5#

这根本不是[]的问题,而是在容器中存储原始指针的问题。

xmq68pz9

xmq68pz96#

如果你的Map像这样:

std::map< int, int* >

字符串
那么你就输了,因为下一个代码片段会泄漏内存:

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


如果处理得当,为什么[]根本不能使用?
如果你正确地测试了你的代码,那么你的代码应该仍然没有通过代码审查,因为你使用了原始指针。
除此之外,如果使用得当,使用map::operator[]没有什么问题。但是,使用insert/find方法可能会更好,因为可能会对map进行静默修改。

wbrvyc0a

wbrvyc0a7#

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

字符串
这里的问题不是map的operator[],而是缺少智能指针。你的指针应该被存储到某个RAII对象中(比如智能指针),它会立即获得分配对象的所有权,并确保它会被释放。
如果你的代码审阅者忽略了这一点,反而说你应该热衷于operator[],那就给他们买一本好的C++教科书。

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


这是真的。但那是因为operator[]被设计成不同的行为。显然,你不应该在它做错事的情况下使用它。

lrl1mhuk

lrl1mhuk8#

一般来说,问题在于operator[]隐式地创建一个与传入的键相关联的值,如果键还没有出现,则在map中插入一个新的对。这可能会从那时起破坏你的逻辑,例如当你搜索某个键是否存在时。

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
}

字符串
我建议只有当你知道你将引用一个已经存在于map中的键时才使用操作符[](顺便说一下,这种情况并不少见)。

ivqmmu1c

ivqmmu1c9#

operator[] of map本身并没有什么问题,只要它的语义符合您的需要即可。(并且知道operator[]的确切语义)。有时候,当条目不存在时,隐式地创建一个具有默认值的新条目正是您想要的(例如,计算文本文档中的单词,其中++ countMap[word]是您所需要的全部);还有许多其他时候不是这样。
代码中一个更严重的问题可能是在Map中存储指针,一个更自然的解决方案可能是使用map <keyType, someClass>,而不是map <keyType, SomeClass*>。例如,我使用了很多map,它们在程序启动时被初始化一次,带有指向静态示例的指针。如果你的map[i] = ...处于初始化循环中,在启动时执行一次,可能没有问题。如果它在代码中的许多不同地方执行,可能会有问题。
这个问题的解决方案不是禁止operator[](或Map到指针)。解决方案是从指定所需的确切语义开始。如果std::map没有直接提供它们(很少提供),那么编写一个小的 Package 器类来定义您想要的确切语义,使用std::map来实现它们。因此,operator[]的 Package 器可能是:

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操作的对象插入到map中的情况。使用newdelete的通常原因是,所涉及的对象有自己的特定生存期(并且是不可复制的--尽管这不是一个绝对的规则,但是如果你正在new一个支持复制和赋值的对象,当Map类型是指针时,要么指向的对象是静态的(并且Map在初始化后或多或少是常量),要么插入和移除是在类的构造函数和析构函数中完成的(但这只是一般规则;当然也有例外)。

相关问题