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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added a comment.

LGTM thanks!!!



================
Comment at: lld/MachO/Driver.cpp:255
 
+    auto file = make<ArchiveFile>(std::move(archive));
     if (config->allLoad || forceLoadArchive) {
----------------
clang-tidy: use `auto *file` pls.


================
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)));
----------------
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?)


================
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)));
----------------
same question here w.r.t erroring twice.

(also the code looks alsmot identical - maybe future clean up could factor them out somehow)


================
Comment at: lld/MachO/InputFiles.cpp:1267-1268
+    return createStringError(inconvertibleErrorCode(),
+                             mb.getBufferIdentifier() +
+                                 " has unhandled file type");
+  }
----------------
could you include the unhandled type in the err msg? (makes it easier to debug when we do get this error)


================
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.
----------------
Why not? Just curious


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108780