[PATCH] D108780: [lld-macho] Refactor archive loading
Jez Ng via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 26 13:24:37 PDT 2021
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
More information about the llvm-commits
mailing list