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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Nice work so far. I think it would be more natural to me if the "main" template argument was that of the underlying container, rather than the iterator. This shouldn't affect the functionality, if I've followed it correctly.

I've got a number of other comments, but don't want to overload things too much in the first instance.



================
Comment at: llvm/include/llvm/Support/UpdatableRange.h:1
+#include "llvm/ADT/SmallVector.h"
+#include <iterator>
----------------
Don't forget to include LLVM copyright headers, even in WIP code.


================
Comment at: llvm/include/llvm/Support/UpdatableRange.h:35
+template <
+    typename T, typename KeyType = uint64_t,
+    typename std::enable_if<
----------------
Is the KeyType type useful in this version?


================
Comment at: llvm/include/llvm/Support/UpdatableRange.h:49
+      Original = 0, // Index represents offset from Start.
+      Removed,      // Index not meaningful.
+      New           // Index represents index into Modified.
----------------
In the current version, you don't need this, since you don't have any way of removing anything.


================
Comment at: llvm/include/llvm/Support/UpdatableRange.h:65
+public:
+  UpdatableRange() = delete;
+  UpdatableRange(const UpdatableRange &) = default;
----------------
Isn't this simply not generated if another constructor was defined?


================
Comment at: llvm/include/llvm/Support/UpdatableRange.h:71
+    Mappings.reserve(Length);
+    for (int I = 0; I < Length; ++I)
+      Mappings.push_back(MappingType(MappingType::Original, I));
----------------
Use size_t.


================
Comment at: llvm/include/llvm/Support/UpdatableRange.h:72
+    for (int I = 0; I < Length; ++I)
+      Mappings.push_back(MappingType(MappingType::Original, I));
+  }
----------------
Rather than this, have you considered using std::transform?


================
Comment at: llvm/include/llvm/Support/UpdatableRange.h:75
+
+  const ValueType &operator[](KeyType Index) {
+    if (Mappings[Index].Type == MappingType::Original)
----------------
Make sure to include comments for each public member function describing what it does.


================
Comment at: llvm/include/llvm/Support/UpdatableRange.h:95
+  ValueType &makeMutable(KeyType Index) {
+    assert(Mappings[Index].Type != MappingType::Removed &&
+           "Element was removed");
----------------
You'll probably want to make this return an Expected or similar, I suspect, rather than assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64462