[PATCH] D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1]

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 6 inline comments as done.
abrachet added a comment.

In D64281#1600516 <https://reviews.llvm.org/D64281#1600516>, @labath wrote:

> Sorry to barge in here, but I couldn't resist to not spread the knowledge of more advanced googletest features. EXPECT_EQ is perfectly safe to use with StringRefs as it just defers to their operator==, and (as I mentioned in the other review), we have better facilities for checking the state of Expected and Error values.


Not at all thanks for the comments I really appreciate them!



================
Comment at: llvm/include/llvm/Object/MutableObject.h:17
+// taking an Iterable::value_type.
+template <typename T> class MutableRange {
+public:
----------------
jhenderson wrote:
> This is the only class in this file, so the file name seems a bit off?
Not sure what to do here. I think eventually I will have a MutableObject base class, and it would seem silly to make a MutableRange.h. What do you think I should do?


================
Comment at: llvm/include/llvm/Object/MutableObject.h:21
+    bool New : 1;
+    uintptr_t Ptr : sizeof(void *) * 8 - 1;
+
----------------
jhenderson wrote:
> Looking at this, this seems incredibly suspicious. If this is supposed to hold a pointer, what's stopping that pointer using the highest bit for memory? Don't worry so much about conserving size here, and also put the two members the other way around (i.e. Ptr before New).
FWIW, I think the MMU's on X86 and AArch64 both only support pointer sizes of 48 bits or similar, in either case less than 64, so pointers cannot be this large. I'm not sure this would be safe for 32 bit architectures though, but we have [[ https://llvm.org/doxygen/classllvm_1_1PointerIntPair.html | PointerIntPair ]] in ADT, so this pattern is used in LLVM.

I would also add that moving the bool out effectively doubles the structs size because presumably the compiler will word align it, but then reading those members is faster now.

In either case it is crazy to worry about something like this in this stage! So I have done away with the bit field.


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

https://reviews.llvm.org/D64281