[PATCH] D113630: Add support for chained fixup load commands to MachOObjectFile

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:3257-3270
+MachOChainedFixupEntry::MachOChainedFixupEntry(Error *E,
+                                               const MachOObjectFile *O,
+                                               FixupKind Kind, bool Parse)
+    : MachOAbstractFixupEntry(E, O), Kind(Kind) {
+  ErrorAsOutParameter e(E);
+  if (Parse) {
+    if (auto FixupTargetsOrErr = O->getDyldChainedFixupTargets())
----------------
JDevlieghere wrote:
> Could this be a static factory method that returns an `Expected<MachOChainedFixupEntry>`? 
Definitely, but I'm trying hard to minimize differences with the downstream code as much as possible. I'm more than happy to refactor the code at the end of patch series though!


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4306-4310
+  MachOChainedFixupEntry Start(&Err, this, Kind, true);
+  Start.moveToFirst();
+
+  MachOChainedFixupEntry Finish(&Err, this, Kind, false);
+  Finish.moveToEnd();
----------------
JDevlieghere wrote:
> If `Start` fails but `Finish` succeeds, won't this trigger the unchecked Error assert? 
Good question! No. Finish is initialized with Parse=false and thus will not overwrite Err. This pattern is used all over the file.


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

https://reviews.llvm.org/D113630