[PATCH] D64281: [Object] Create MutableObject class for doing Object Mutations

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You should add doxygen style comments for all the new functions/methods you are adding.

You should add some tests to a new file in the unit tests for libObject, that exercises the very basic functionality of creating and fetching data from an UpdatableList and MutableObject.

There might be an argument for moving UpdatableList into the support library, if it can be made more generic.



================
Comment at: llvm/include/llvm/Object/MutableObject.h:20-24
+#include <vector>
+#include <unordered_map>
+#include <memory>
+#include <set>
+#include <functional>
----------------
Have you run clang-format on this file? Usually, I think it orders the headers alphabetically.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:28
+namespace object {
+namespace mutable_object {
+
----------------
You probably don't need the extra level of namespacing. The object namespace is probably sufficient.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:30
+
+/// Leaves an original vector of Entity's immutable but provides ways to
+/// create new Entity's and allow mutations of the new ones. This makes it 
----------------
plural of Entity is Entities. Honestly, I think you can just call this T though, just like std::vector etc.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:34
+template <typename Entity>
+class UpdatableList {
+public:
----------------
Perhaps it's worth making this more generic: call this class UpdatableRange, and template it on the original container type too, then consider using iterators as the constructor arguments. That would avoid needing to maintain the underlying std::vector. Alternatively, pass the underlying container in as a template parameter.

Essentially I think this container makes sense as a kind of container facade, effectively making a const container non-const. Therefore, it makes most sense if this provides a generic container-like interface as much as possible, and is useful (no need to implement container functions that have no purpose).


================
Comment at: llvm/include/llvm/Object/MutableObject.h:37
+  using Ptr = std::unique_ptr<Entity>;
+  using OriginalType = std::vector<Ptr>;
+private:
----------------
I don't think this `using` is worth it.

Nit: blank line before `private:`


================
Comment at: llvm/include/llvm/Object/MutableObject.h:42
+
+  // Represents the largest number of Entity's in the list This is used for
+  // createNew which will insert into the map with a key of CurrentNum++.
----------------
list This -> list. This


================
Comment at: llvm/include/llvm/Object/MutableObject.h:44
+  // createNew which will insert into the map with a key of CurrentNum++.
+  uint64_t CurrentNum = 0;
+public:
----------------
CurrentNum is a bit ambiguous. Perhaps Size would be more appropriate?


================
Comment at: llvm/include/llvm/Object/MutableObject.h:46
+public:
+  UpdatableList() = default;
+  UpdatableList(const UpdatableList &) = delete;
----------------
Does default constructing this make sense?


================
Comment at: llvm/include/llvm/Object/MutableObject.h:48
+  UpdatableList(const UpdatableList &) = delete;
+  //UpdatableList(UpdatableList &&) = default;
+  UpdatableList(OriginalType Original)
----------------
Don't leave commented out code in patches. Either delete this line or put it in.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:49
+  //UpdatableList(UpdatableList &&) = default;
+  UpdatableList(OriginalType Original)
+  : Original(std::move(Original)), CurrentNum(Original.size()) {}
----------------
See note above: it might make more sense for this to take a pair of iterators as constructor arguments. Also, use `explicit` for single-argument constructor.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:52
+
+  const Entity &operator[](uint64_t Index) const {
+    auto Found = Updated.find(Index);
----------------
You might want to template the Index type as a KeyType, making it more generic. That allows containers other than sequence containers like std::vector.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:55
+    if (Found != Updated.end()) {
+      assert(Found->second.get() && "Section was deleted");
+      return *Found->second.get();
----------------
"Section" -> "Element" or similar.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:61
+
+  // Original sections are immutable, return them as const
+  // this is the same as operator[](uint64_t) for original indexes.
----------------
sections -> entities

But I don't think you really need this comment. Instead, describe what this is actually doing, i.e. something like "returns the member at Index in the original container, regardless of modifications."


================
Comment at: llvm/include/llvm/Object/MutableObject.h:64
+  const Entity &getOriginal(uint64_t Index) const {
+    assert(Index < Original.size() && "Out of range");
+    return *Original[Index].get();
----------------
Should this be an llvm:Expected, rather than an assert? The replace function emits an Error in this case.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:68-70
+  // Sections that have been updated should not be immutable
+  // so return them as non const. If a section has not yet been created as
+  // mutable, create it and return the newly created mutable Entity.
----------------
Similar comments to getOriginal.

I'm not convinced this function should exist, especially in the public interface. It might help if you put up the next patch or two, to show how these functions are going to be used. I'd expect users to call replace() or similar.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:81
+
+  Error replace(uint64_t Index, Ptr New) {
+    if (Updated.find(Index) == Updated.end()) {
----------------
I wonder if it might make sense to omit this and the remove and addNew functions in the first version. In the first version, you could just have effectively a wrapper around Object, that can access the underlying data.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:84
+      if (Index >= Original.size())
+        return createStringError(errc::argument_out_of_domain,
+                                 "Updating section that doesn't exist");
----------------
I don't think argument_out_of_domain is the correct error here, as this isn't a mathematical function. I'd just go with invalid_argument.

Also the error message should not refer to sections.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:97
+  // returns a reference to it.
+  Entity &addNew() {
+    const Entity &First = getOriginal(0);
----------------
I'm not sure this function makes sense. Why should it clone the first member to add a new one at the end? I think you probably only want the second overload.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:103
+
+  template <class... Ts> Entity &addNew(Ts &&... Args) {
+    Updated[CurrentNum] = make_unique<Entity>(std::forward<Ts>(Args)...);
----------------
It might be nice if this took an index/iterator/key to insert at (and call this function `insert`).


================
Comment at: llvm/include/llvm/Object/MutableObject.h:104
+  template <class... Ts> Entity &addNew(Ts &&... Args) {
+    Updated[CurrentNum] = make_unique<Entity>(std::forward<Ts>(Args)...);
+    return *Updated.find(CurrentNum++)->second;
----------------
You might want to explicit call this llvm::make_unique, to avoid clashing with std::make_unique where it's supported.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:113
+class MutableObject {
+protected:
+  using SectionList = UpdatableList<SectionBase>;
----------------
Make this private until a sub-class exists that needs the state.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:114-115
+protected:
+  using SectionList = UpdatableList<SectionBase>;
+  using SegmentList = UpdatableList<SegmentBase>;
+
----------------
Move these to next to the state that use them.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:120
+  SectionList Sections;
+  SegmentList Segments;
+
----------------
It might make sense to add Symbols here too.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:124
+public:
+  virtual ~MutableObject() {}
+
----------------
You don't need this yet, since you have no sub-classes yet.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:131-134
+  Error removeSection(uint64_t Section) { Sections.remove(Section); }
+  Error replaceSection(uint64_t ToReplace, std::unique_ptr<SectionBase> New) {
+    Sections.replace(ToReplace, std::move(New));
+  }
----------------
I'd be inclined to get rid of these in this first version.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:136-137
+
+  SectionBase &getMutableSection(uint64_t Section) { Sections.getUpdated(Section); }
+  SegmentBase &getMutableSegment(uint64_t Segment) { Segments.getUpdated(Segment); }
+};
----------------
I think the more useful thing in the first version is to provide basic accessors that get the segment (modified or original).


================
Comment at: llvm/lib/Object/MutableObject.cpp:1
+//===- MutableObject.h - An object file which can be mutated ----*- C++ -*-===//
+//
----------------
Don't forget to clang-format this file. You also need to update this line for the .cpp file.


================
Comment at: llvm/lib/Object/MutableObject.cpp:9
+//
+// This class represents an object file which can be mutated, then recreated
+// to reflect the changes.
----------------
I don't think you need this blurb in both .h and .cpp.


================
Comment at: llvm/lib/Object/MutableObject.cpp:14
+
+#include "llvm/Object/MutableObject.h"
+#include "llvm/Object/ObjectFile.h"
----------------
I'd be surprised if you need any include other than this one.


================
Comment at: llvm/lib/Object/MutableObject.cpp:32
+
+  return createStringError(errc::not_supported, "Unkown filetype");
+}
----------------
Unkown -> Unknown

I'd actually be inclined to let this do nothing in the first instance, rather than report an error. That way you can write tests that use it usefully.


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

https://reviews.llvm.org/D64281