[PATCH] D64462: [ADT] [WIP] Add MutableRange class

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ADT/MutableRange.h:52
+  /// by Index.
+  struct alignas(sizeof(void *)) MappingType {
+    enum Operation {
----------------
Why is the alignas needed?


================
Comment at: llvm/include/llvm/ADT/MutableRange.h:67-68
+
+  const Iter First;
+  const Iter Last;
+
----------------
Begin and End? Probably even UnderlyingBegin and UnderlyingEnd to disambiguate


================
Comment at: llvm/include/llvm/ADT/MutableRange.h:90
+      : MutableRange(Container.begin(), Container.end()) {}
+  MutableRange(Iter First, Iter Last) : First(First), Last(Last) {
+    SizeType Length = std::distance(First, Last);
----------------
Begin and End. Don't use "Last", as it could be thought to be the last valid member (i.e. one before the end iterator).


================
Comment at: llvm/include/llvm/ADT/MutableRange.h:101
+
+  /// Returns an immutable reference to object at Index.
+  const ValueType &operator[](SizeType Index) const {
----------------
"to the object"


================
Comment at: llvm/include/llvm/ADT/MutableRange.h:107
+  /// Emplaces a T::value_type at a specified index.
+  template <class... Ts> ValueType &insert(SizeType Index, Ts &&... Args) {
+    Modified.emplace_back(ValueType(std::forward<Ts>(Args)...));
----------------
I would say that it's more common for insert to take a value_type to insert, rather than a variadic template list to construct it. I don't mind this method existing as is, but I think you should provide the "standard" signature too.


================
Comment at: llvm/include/llvm/ADT/MutableRange.h:115
+
+  /// Appends new element to the back.
+  template <class... Ts> ValueType &emplace_back(Ts &&... Args) {
----------------
"a new element"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64462