[PATCH] D53247: [ADT] Add initializer_list constructor, equality tests for DenseMap/DenseSet.

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Oct 14 12:35:45 PDT 2018


dblaikie added inline comments.


================
Comment at: include/llvm/ADT/DenseMap.h:656-659
+  for (auto LHSI = LHS.begin(), RHSI = RHS.begin(), E = LHS.end(); LHSI != E;
+       ++LHSI, ++RHSI)
+    if (*LHSI != *RHSI)
+      return false;
----------------
lhames wrote:
> lhames wrote:
> > dblaikie wrote:
> > > Is this guaranteed to work, given the lack of ordering guarantees? I think the order can depend on insertion order - so equivalent containers might have different iteration orders, perhaps?
> > Good point. No -- this won't work.
> > 
> > This was mostly a convenience for unit tests, so dropping it (at the cost of some slightly more verbose unit tests) seems like the best solution.
> Alternatively they could be implemented by iterating over one structure and testing that the corresponding element is present in the other. In either case I think I'll want some extra comments for them, so I will break the equality tests out into a separate patch. 
Yeah, sounds pretty reasonable to me.

Yeah, looks like the standard containers have similar complexity (amortized linear (since the lookups are amortized constant), worst case N^2 (if all the elements hash collide))

"Proportional to N calls to operator== on value_type, calls to the predicate returned by key_eq, and calls to the hasher returned by hash_function, in the average case, proportional to N2 in the worst case where N is the size of the container." - https://en.cppreference.com/w/cpp/container/unordered_set/operator_cmp


Repository:
  rL LLVM

https://reviews.llvm.org/D53247





More information about the llvm-commits mailing list