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

[Alex Brachet via Phabricator via llvm-commits]

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

Thanks for the comments @grimar!



================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:103
+      NewValues.push_back(New);
+      Mappings.push_back(MappingType(NewValues.size() - 1, MappingType::New));
+    }
----------------
grimar wrote:
> ```
> Mappings.push_back({NewValues.size() - 1, MappingType::New});
> ```
I'll do you one better, `emplace_back` :)


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:197
+               : DynSymbols;
+  }
+
----------------
grimar wrote:
> What about implementing `const` version using `non-const`? I.e. call `getWhichTable` from `getWhichTable const`.
How do I do this without const_casting `this`? Is that how you do it? This is very useful because this duplicating methods like this has been bugging me recently. Thanks!


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:263
+  /// Removes either a dynamic symbol or regular symbol.
+  void removeSymbol(symbol_iterator Sym) {
+    MutableSymbolTable &SymTab = getWhichTable(Sym->getRawDataRefImpl());
----------------
grimar wrote:
> I think when you have a symbol iterator, you usually know if it is a regular or dynamic
> symbol. I am not sure it worth to allow mixing them. I.e. I am thinking about making 2 different functions,
> like `removeSymbol` and `removeDynamicSymbol`.
> 
> I am not feeling strong here, though it seems to be less error prone.
I have added these, I don't feel strongly either. FWIW, all of the `getSymbol*` methods in ELFObjectFile work on either symbol type so I duplicated that here. I'll keep this as not done so others who have an opinion stronger than ours can chime in.


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

https://reviews.llvm.org/D65367