Switch base::IDMap to use unique_ptr.

base/id_map.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_ID_MAP_H_
 #define BASE_ID_MAP_H_
 
 #include <stddef.h>
 #include <stdint.h>
+#include <memory>
 #include <set>
+#include <type_traits>
+#include <utility>
 
 #include "base/containers/hash_tables.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/sequence_checker.h"
 
 // Ownership semantics - own pointer means the pointer is deleted in Remove()
 // & during destruction
 enum IDMapOwnershipSemantics {
   IDMapExternalPointer,
@@ skipping 12 matching lines @@
 // This class does not have a virtual destructor, do not inherit from it when
 // ownership semantics are set to own because pointers will leak.
 template <typename T,
           IDMapOwnershipSemantics OS = IDMapExternalPointer,
           typename K = int32_t>
 class IDMap {
  public:
   using KeyType = K;
 
  private:
-  typedef base::hash_map<KeyType, T*> HashTable;
+  typedef typename std::conditional<OS == IDMapExternalPointer,

[danakj, 2016/09/15 20:08:40]

can you change these both to "using" instead of typedef

[aelias_OOO_until_Jul13, 2016/09/15 21:09:35]

Done.

+                                  T*,
+                                  std::unique_ptr<T>>::type V;
+  typedef base::hash_map<KeyType, V> HashTable;
 
  public:
   IDMap() : iteration_depth_(0), next_id_(1), check_on_null_data_(false) {
     // A number of consumers of IDMap create it on one thread but always
     // access it from a different, but consistent, thread (or sequence)
     // post-construction. The first call to CalledOnValidSequence() will re-bind
     // it.
     sequence_checker_.DetachFromSequence();
   }
 
   ~IDMap() {
     // Many IDMap's are static, and hence will be destroyed on the main
     // thread. However, all the accesses may take place on another thread (or
     // sequence), such as the IO thread. Detaching again to clean this up.
     sequence_checker_.DetachFromSequence();
-    Releaser<OS, 0>::release_all(&data_);
   }
 
   // Sets whether Add and Replace should DCHECK if passed in NULL data.
   // Default is false.
   void set_check_on_null_data(bool value) { check_on_null_data_ = value; }
 
   // Adds a view with an automatically generated unique ID. See AddWithID.
   KeyType Add(T* data) {

[danakj, 2016/09/15 20:08:40]

Can this not take a V instead?

[aelias_OOO_until_Jul13, 2016/09/15 21:09:35]

Yes, it can and should, but it would require expanding the scope of the patch to
touch ~100 files and I wanted initial reviewer feedback before embarking on
that.

This patch has a small risk of affecting perfbots or correctness and getting
reverted, so I'm thinking it would make sense to land this small one first, then
do the big no-op boilerplatey one once this proves to stick?  WDYT?  I added a
TODO(aelias) to change to Vs.

[danakj, 2016/09/16 00:30:27]

Cool cool. Can you add a V version now and the comment on the T* one say to not
use it and use the V version?

[aelias_OOO_until_Jul13, 2016/09/16 01:39:18]

Done.  Note that because V can be the same as T*, I had to add an explicit
unique_ptr<T> variant and make the V variant a private method.

     DCHECK(sequence_checker_.CalledOnValidSequence());
     DCHECK(!check_on_null_data_ || data);
     KeyType this_id = next_id_;
     DCHECK(data_.find(this_id) == data_.end()) << "Inserting duplicate item";
-    data_[this_id] = data;
+    data_[this_id] = V(data);
     next_id_++;
     return this_id;
   }
 
   // Adds a new data member with the specified ID. The ID must not be in
   // the list. The caller either must generate all unique IDs itself and use
   // this function, or allow this object to generate IDs and call Add. These
   // two methods may not be mixed, or duplicate IDs may be generated
   void AddWithID(T* data, KeyType id) {

[danakj, 2016/09/15 20:08:40]

And this?

     DCHECK(sequence_checker_.CalledOnValidSequence());
     DCHECK(!check_on_null_data_ || data);
     DCHECK(data_.find(id) == data_.end()) << "Inserting duplicate item";
-    data_[id] = data;
+    data_[id] = V(data);
   }
 
   void Remove(KeyType id) {
     DCHECK(sequence_checker_.CalledOnValidSequence());
     typename HashTable::iterator i = data_.find(id);
     if (i == data_.end()) {
       NOTREACHED() << "Attempting to remove an item not in the list";
       return;
     }
 
     if (iteration_depth_ == 0) {
-      Releaser<OS, 0>::release(i->second);
       data_.erase(i);
     } else {
       removed_ids_.insert(id);
     }
   }
 
-  // Replaces the value for |id| with |new_data| and returns a pointer to the
-  // existing value. If there is no entry for |id|, the map is not altered and
-  // nullptr is returned. The OwnershipSemantics of the map have no effect on
-  // how the existing value is treated, the IDMap does not delete the existing
-  // value being replaced.
-  T* Replace(KeyType id, T* new_data) {
+  // Replaces the value for |id| with |new_data| and returns the existing value
+  // (as a unique_ptr<> if in IDMapOwnPointer mode).  Should only be called with
+  // an already added id.
+  V Replace(KeyType id, V new_data) {
     DCHECK(sequence_checker_.CalledOnValidSequence());
     DCHECK(!check_on_null_data_ || new_data);
     typename HashTable::iterator i = data_.find(id);
-    if (i == data_.end()) {
-      NOTREACHED() << "Attempting to replace an item not in the list";
-      return nullptr;
-    }
+    DCHECK(i != data_.end());
 
-    T* temp = i->second;
-    i->second = new_data;
-    return temp;
+    std::swap(i->second, new_data);
+    return new_data;
   }
 
   void Clear() {
     DCHECK(sequence_checker_.CalledOnValidSequence());
     if (iteration_depth_ == 0) {
-      Releaser<OS, 0>::release_all(&data_);
+      data_.clear();
     } else {
       for (typename HashTable::iterator i = data_.begin();
            i != data_.end(); ++i)
         removed_ids_.insert(i->first);
     }
   }
 
   bool IsEmpty() const {
     DCHECK(sequence_checker_.CalledOnValidSequence());
     return size() == 0u;
   }
 
   T* Lookup(KeyType id) const {
     DCHECK(sequence_checker_.CalledOnValidSequence());
     typename HashTable::const_iterator i = data_.find(id);
     if (i == data_.end())
-      return NULL;
-    return i->second;
+      return nullptr;
+    return &*i->second;
   }
 
   size_t size() const {
     DCHECK(sequence_checker_.CalledOnValidSequence());
     return data_.size() - removed_ids_.size();
   }
 
 #if defined(UNIT_TEST)
   int iteration_depth() const {
     return iteration_depth_;
@@ skipping 40 matching lines @@
       return iter_ == map_->data_.end();
     }
 
     KeyType GetCurrentKey() const {
       DCHECK(map_->sequence_checker_.CalledOnValidSequence());
       return iter_->first;
     }
 
     ReturnType* GetCurrentValue() const {
       DCHECK(map_->sequence_checker_.CalledOnValidSequence());
-      return iter_->second;
+      return &*iter_->second;
     }
 
     void Advance() {
       DCHECK(map_->sequence_checker_.CalledOnValidSequence());
       ++iter_;
       SkipRemovedEntries();
     }
 
    private:
     void Init() {
@@ skipping 11 matching lines @@
     }
 
     IDMap<T, OS, K>* map_;
     typename HashTable::const_iterator iter_;
   };
 
   typedef Iterator<T> iterator;
   typedef Iterator<const T> const_iterator;
 
  private:
-
-  // The dummy parameter is there because C++ standard does not allow
-  // explicitly specialized templates inside classes
-  template<IDMapOwnershipSemantics OI, int dummy> struct Releaser {
-    static inline void release(T* ptr) {}
-    static inline void release_all(HashTable* table) {}
-  };
-
-  template<int dummy> struct Releaser<IDMapOwnPointer, dummy> {
-    static inline void release(T* ptr) { delete ptr;}
-    static inline void release_all(HashTable* table) {
-      for (typename HashTable::iterator i = table->begin();
-           i != table->end(); ++i) {
-        delete i->second;
-      }
-      table->clear();
-    }
-  };
-
   void Compact() {
     DCHECK_EQ(0, iteration_depth_);
     for (const auto& i : removed_ids_)
       Remove(i);
     removed_ids_.clear();
   }
 
   // Keep track of how many iterators are currently iterating on us to safely
   // handle removing items during iteration.
   int iteration_depth_;
@@ skipping 10 matching lines @@
 
   // See description above setter.
   bool check_on_null_data_;
 
   base::SequenceChecker sequence_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(IDMap);
 };
 
 #endif  // BASE_ID_MAP_H_