[PATCH] D88113: [llvm-objcopy][NFC] refactor error handling. part 1.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 23 01:21:17 PDT 2020
jhenderson added inline comments.
================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:239-242
+ if (Expected<elf::ELFCopyConfig> ELFConfig = elf::parseConfig(*this))
+ ELF = *ELFConfig;
+ else
return ELFConfig.takeError();
----------------
Why did this code change? It looks semantically identical, and I think it's actually a little less readable.
================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:358
MachOReader Reader(In);
- std::unique_ptr<Object> O = Reader.create();
- if (!O)
- return createFileError(
- Config.InputFilename,
- createStringError(object_error::parse_failed,
- "unable to deserialize MachO object"));
-
- if (Error E = handleArgs(Config, *O))
- return createFileError(Config.InputFilename, std::move(E));
-
- // Page size used for alignment of segment sizes in Mach-O executables and
- // dynamic libraries.
- uint64_t PageSize;
- switch (In.getArch()) {
- case Triple::ArchType::arm:
- case Triple::ArchType::aarch64:
- case Triple::ArchType::aarch64_32:
- PageSize = 16384;
- break;
- default:
- PageSize = 4096;
- }
+ if (Expected<std::unique_ptr<Object>> O = Reader.create()) {
+ if (!*O)
----------------
Cleaner code would be to just do:
```
Expected<std::unique_ptr<Object>> O = Reader.create();
if (!ObjOrErr)
return O.takeError();
...
```
================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:360-363
+ return createFileError(
+ Config.InputFilename,
+ createStringError(object_error::parse_failed,
+ "unable to deserialize MachO object"));
----------------
If I'm following things correctly, this piece of code is dead? Even if there were errors in the `create` method, they should now be reported via the `Expected` being returned. If it is dead, please remove it. If not, I think we should change the behaviour to report the error in the same manner as the other errors.
================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.h:27-42
+inline Error errorWithContext(StringRef Context, Error E) {
+ assert(E);
+ std::string Buf;
+ raw_string_ostream OS(Buf);
+ Optional<std::error_code> EC;
+ handleAllErrors(std::move(E), [&](const ErrorInfoBase &EI) {
+ OS << "'" + Context + "': ";
----------------
If I'm not mistaken, this whole function can just be replaced with a single `createStringError()` call, similar to what we often do in llvm-readobj (see e.g. https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-readobj/ELFDumper.cpp#L5348).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88113/new/
https://reviews.llvm.org/D88113
More information about the llvm-commits
mailing list