Part 2 of base::IDMap refactor to eliminate IDMapOwnPointer/IDMapExternalPointer modes

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:
-// - OwnPointer means we store a unique_ptr that owns the object (so the
-//   object is deleted in Remove() and during destruction).
-// - ExternalPointer means we store a raw pointer and don't own the object
-
-// TODO (http://crbug.com/647091): eliminate this enum, replace OwnPointer
-// mode in callsites with IDMap<unique_ptr<T>>
-enum IDMapOwnershipSemantics {
-  IDMapExternalPointer,
-  IDMapOwnPointer
-};
-
 // This object maintains a list of IDs that can be quickly converted to
 // pointers to objects. It is implemented as a hash table, optimized for
 // relatively small data sets (in the common case, there will be exactly one
 // item in the list).
 //
 // Items can be inserted into the container with arbitrary ID, but the caller
 // must ensure they are unique. Inserting IDs and relying on automatically
 // generated ones is not allowed because they can collide.
 //
 // This class does not have a virtual destructor, do not inherit from it when

[danakj, 2016/11/30 23:02:21]

This comment doesn't quite make sense anymore. Does anything inherit from this?
That'd be crazy. Can you maybe just mark the class as final instead?

[rlanday, 2016/12/01 00:50:07]

Yeah it looks like nothing inherits from this

 // ownership semantics are set to own because pointers will leak.
-template <typename T,
-          IDMapOwnershipSemantics OS = IDMapExternalPointer,
-          typename K = int32_t>
+template <typename V, typename K = int32_t>

[danakj, 2016/11/30 23:02:21]

Maybe worth a comment now saying that the values (represented as the V
parameter) can be any type that can be dereferenced, such as a raw pointer or a
smart pointer.

[rlanday, 2016/12/01 00:50:07]

Ok, that's a good point

 class IDMap {
  public:
   using KeyType = K;
 
  private:
-  using V = typename std::
-      conditional<OS == IDMapExternalPointer, T*, std::unique_ptr<T>>::type;
+  using T = typename std::remove_reference<decltype(*V())>::type;
   using HashTable = base::hash_map<KeyType, V>;
 
  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();
   }
@@ skipping 79 matching lines @@
   int iteration_depth() const {
     return iteration_depth_;
   }
 #endif  // defined(UNIT_TEST)
 
   // It is safe to remove elements from the map during iteration. All iterators
   // will remain valid.
   template<class ReturnType>
   class Iterator {
    public:
-    Iterator(IDMap<T, OS, K>* map)
-        : map_(map),
-          iter_(map_->data_.begin()) {
+    Iterator(IDMap<V, K>* map) : map_(map), iter_(map_->data_.begin()) {
       Init();
     }
 
     Iterator(const Iterator& iter)
         : map_(iter.map_),
           iter_(iter.iter_) {
       Init();
     }
 
     const Iterator& operator=(const Iterator& iter) {
@@ skipping 43 matching lines @@
     }
 
     void SkipRemovedEntries() {
       while (iter_ != map_->data_.end() &&
              map_->removed_ids_.find(iter_->first) !=
              map_->removed_ids_.end()) {
         ++iter_;
       }
     }
 
-    IDMap<T, OS, K>* map_;
+    IDMap<V, K>* map_;
     typename HashTable::const_iterator iter_;
   };
 
   typedef Iterator<T> iterator;
   typedef Iterator<const T> const_iterator;
 
  private:
   KeyType AddInternal(V data) {
     DCHECK(sequence_checker_.CalledOnValidSequence());
     DCHECK(!check_on_null_data_ || data);
@@ skipping 34 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_