[PATCH] D108780: [lld-macho] Refactor archive loading

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Driver.cpp:261-267
+          if (Error e = file->fetch(c, reason))
+            error(toString(file) + ": " + reason +
+                  " failed to load archive member: " + toString(std::move(e)));
         }
+        if (e)
+          error(toString(file) +
+                ": Archive::children failed: " + toString(std::move(e)));
----------------
oontvoo wrote:
> We've already done `error()` on line 262, why do we need to do it again on 266?
> (Wouldn't this error on the last error twice?)
They're different errors -- one error is from fetching, the other one from trying to get the children


================
Comment at: lld/MachO/Driver.cpp:283-289
+          if (Error e = file->fetch(c, "-ObjC"))
+            error(toString(file) + ": -ObjC failed to load archive member: " +
+                  toString(std::move(e)));
         }
+        if (e)
+          error(toString(file) +
+                ": Archive::children failed: " + toString(std::move(e)));
----------------
oontvoo wrote:
> same question here w.r.t erroring twice.
> 
> (also the code looks alsmot identical - maybe future clean up could factor them out somehow)
I guess we could have a helper function that just extracts the children and prints the error, but in general I think this code is ugly because the underlying Archive API is ugly


================
Comment at: lld/MachO/InputFiles.h:190
+  void fetch(const llvm::object::Archive::Symbol &);
+  // LLD normally doesn't use Error for error-handling, but the underlying
+  // Archive library does, so this is the cleanest way to wrap it.
----------------
oontvoo wrote:
> Why not? Just curious
we usually just dump the error to stderr directly via error() or fatal(), so there's no need to return it to the caller


================
Comment at: lld/MachO/InputFiles.h:193
+  Error fetch(const llvm::object::Archive::Child &, StringRef reason);
+  llvm::object::Archive &getArchive() const { return *file; };
 
----------------
oontvoo wrote:
> Does this want to return a const ref instead?
yeah we can do that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108780