[PATCH] D65367: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 2]

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Not had time to look at everything in this patch, but it feels like you don't want removal functionality to be added in this patch at the same time as basic symbol functionality. That should be a different patch, and can then include both symbol and section removal. What do you think?

By the way, the overall approach with removal seems sound.



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:71
+  /// to preserve the index of all other other entries, similarly insertion is
+  /// not supported and only the equivalent to push_back, add.
+  ///
----------------
"and only the equivalent to push_back, add." sounds like an unfinished sentence to me.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:116-117
 
+    /// operator[] should not do this work because otherwise indexes would
+    /// effectively be clobbered. Uses a default of UINT64_MAX so that it
+    /// overflows to 0 to get the first element.
----------------
This comment doesn't document what the function does. It talks about class implementation details, which doesn't really make sense.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:119-120
+    /// overflows to 0 to get the first element.
+    uint64_t getNextIndex(
+        uint64_t CurrentIndex = std::numeric_limits<uint64_t>::max()) const {
+      /// <= so that Index can reach an end iterator state.
----------------
I'm inclined to think that `getNextIndex` shouldn't provide you the first index under any circumstance. You have `getFirstNonRemoved` instead.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:121
+        uint64_t CurrentIndex = std::numeric_limits<uint64_t>::max()) const {
+      /// <= so that Index can reach an end iterator state.
+      while (Mappings[++CurrentIndex].Type == MappingType::Removed &&
----------------
No need for doxygen-style comments in the middle of a function.

Your opening comment should however explain what happens when the last index is passed in/reached by the loop.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:128
+
+    uint64_t getRelativeIndex(uint64_t Index) const {
+      uint64_t Ret = 0;
----------------
Comment?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:136
+
+    uint64_t getFirstNotRemoved() const {
+      for (uint64_t I = 0;; ++I)
----------------
How about getFirstIndex, to match getNextIndex?

Also this function needs a comment.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:137
+    uint64_t getFirstNotRemoved() const {
+      for (uint64_t I = 0;; ++I)
+        if (Mappings[I].Type != MappingType::Removed || I == end())
----------------
I'd put the I == end() clause in here, rather than in the body of the loop and just return I instead of the llvm_unreachable statement.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:153
+    void remove(uint64_t Index) {
+      assert(Index < OriginalValues.size() && "Out of bounds");
+      Mappings[Index].Type = MappingType::Removed;
----------------
Assert that the Index hasn't already been removed.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:157
+
+    /// Adds a NewType to the back of the list.
+    void add(NewType &&New) {
----------------
This comment should refer to the parameter e.g. "Adds \param New to the back of the list."


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:158
+    /// Adds a NewType to the back of the list.
+    void add(NewType &&New) {
+      NewValues.push_back(New);
----------------
Don't use "New" as a variable name, since it won't work with the upcoming change in variable naming rules.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:188
+    /// because size returns the number of valid (non removed) entries.
+    size_t end() const { return Mappings.size(); }
+
----------------
I'd call this lastIndex, as this doesn't return an iterator.

element, this -> element. This


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

https://reviews.llvm.org/D65367