[PATCH] D64462: [Support] [WIP] Add UpdatableRange class

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

If you haven't already, it's worth looking at the other ADT classes like SmallVector to make sure your code matches the same style etc used there.

I think the name "MutableRange" might be more consistent with naming schemes within LLVM.

Overall, I quite like this. I think it's at the point where it's worth getting a wider audience by adding some more people as reviewers.



================
Comment at: llvm/include/llvm/ADT/UpdatableRange.h:1
+//===--- UpdatableRange.h - Switch-on-literal-string Construct ------------===/
+//
----------------
Minor point this but I believe this needs to mention c++ somewhere so that editors can detect the language properly. I'm also not sure the rest of the text makes much sense... ;)


================
Comment at: llvm/include/llvm/ADT/UpdatableRange.h:55
+      New           /// Index represents index into Modified.
+                    /// Is either an addition or replacment.
+    };
----------------
Nit: replacment -> replacement


================
Comment at: llvm/include/llvm/ADT/UpdatableRange.h:87-88
+  UpdatableRange(UpdatableRange &&) = default;
+  UpdatableRange(const T &Container)
+  : UpdatableRange(Container.begin(), Container.end()) {}
+  UpdatableRange(Iter First, Iter Last)
----------------
Nit: clang-format?


================
Comment at: llvm/include/llvm/ADT/UpdatableRange.h:122
+  void erase(SizeType Index) {
+    assert(Index < Mappings.size() && "Index larger than number of elements");
+    Mappings.erase(Mappings.begin() + Index);
----------------
abrachet wrote:
> Should this return Error?
Actually, looking at SmallVector, it looks like I was wrong to encourage you to use Expected below (it uses asserts too), so this should remain an assert.


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

https://reviews.llvm.org/D64462