[PATCH] D63185: [llvm-objcopy] [WIP] Librarify llvm-objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:1
+#ifndef LLVM_OBJECT_MUTABLEELFOBJECT_H
+#define LLVM_OBJECT_MUTABLEELFOBJECT_H
----------------
Even though this is WIP, you should probably include the LLVM license at the top of the new files, as this is technically an LLVM contribution already.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:20
+  ELFSection(const MutableObject &OwningObject,
+             ArrayRef<uint8_t> Data, StringRef Name, Elf_Shdr_Base<ELFT> &Shdr)
+  : SectionBase(OwningObject, Data, Name), SectionHeader(Shdr) {}
----------------
Is there a particular reason this takes both a Name and a section header?


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:46
+    auto *Ret = dyn_cast<ELFObjectFile<ELFT>>(ObjFile);
+    assert(Ret);
+    return *Ret;
----------------
FWIW, I don't think this assert adds anything.


================
Comment at: llvm/lib/Object/CMakeLists.txt:18
   ModuleSymbolTable.cpp
+  MutableELFObject.cpp
+  MutableObject.cpp
----------------
This is causing me a problem on my Windows machine at least - CMake doesn't like it when there's no such file. Did you forget to add it?


================
Comment at: llvm/lib/Object/MutableObject.cpp:56-62
+MutableObjectRange<SectionBase> MutableObject::sections() const {
+  return MutableObjectRange(Sections);
+}
+
+MutableObjectRange<SegmentBase> MutableObject::segments() const {
+  return MutableObjectRange<SegmentBase>(Segments);
+}
----------------
abrachet wrote:
> I need help here. I just can't get it to compile. I guess I'm not understanding templates correctly? The constructor looks like this `MutableObjectRange<T>::MutableObjectRange(UpdadableList<T>)`. I want the compiler to deduce the template parameter to type `T` when I pass an `UpdadableList<T>`. Either way even if I declare the template parameter like I do on line 61, it still doesn't compile. Any tips?
MutableObjectRange's constructor takes a non-const reference: `MutableObjectRange(UpdatableList<Iterable> &List);`. However, `sections()` and `segments()` are listed as const methods, which makes the members of `MutableObject` (i.e. `Sections` and `Segments`) const, and therefore the input arguments don't match up with the signature (passing in a `const &` when a plain `&` is expected. Changing the signature to `const &` was enough ot fix this issue.

You also need to be explicit about the template you are using, as MutableObjectRange is a type. It's the same as a std::vector for example. You wouldn't expect the following to compile, even though it could deduce the constructor argument: `return std::vector(1234)`. Methods can have template argument deduction, but not types. Thus if the constructor itself was templated, you might reasonable expect the template arguments to be deducible.


================
Comment at: llvm/lib/Object/MutableObject.cpp:65
+SectionBase &MutableObject::getMutableSection(DataRef Section) {
+  Sections.getNew(Section);
+}
----------------
This and getMutableSegment aren't returning a value.


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

https://reviews.llvm.org/D63185