[PATCH] D121817: [llvm][ADT] Add a method to MapVector for erasing a range of elements.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

This reduces the total cost of the erase from O(N*M), where N is the total number of elements in the map and M is the number of elements you're removing, to O(N). That still seems like a significant performance hazard.  I think we should discourage this sort of usage, in favor of a container where the removal is O(M), like std::map.

That said, depending on the usage, that might be okay, so I guess I'm not strongly against this.



================
Comment at: llvm/include/llvm/ADT/MapVector.h:207
+    difference_type NumElts = std::distance(S, E);
+    assert(NumElts > 0 && "Range end is not greater than range start!");
+    for (auto I = S; I != E; ++I)
----------------
Asserting that you can't erase an empty range seems weird to me; standard library containers treat it as a no-op.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121817/new/

https://reviews.llvm.org/D121817