[PATCH] D101835: [llvm] llvm-tapi-diff

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added a comment.

Hey, just a few notes on proper error handling.



================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:40
+                        (FileName + ": "));
+}
+
----------------
The idiomatic solution for this is `ExitOnErr` as described here:
https://llvm.org/docs/ProgrammersManual.html#using-exitonerror-to-simplify-tool-code


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:52
+    return BinaryOrErr;
+  return std::move(*BinaryOrErr);
+}
----------------
Could the result be returned directly here? Like this?
```
return createBinary(BufferOrErr.get()->getMemBufferRef());
```


================
Comment at: llvm/tools/llvm-tapi-diff/llvm-tapi-diff.cpp:73
+    return EXIT_FAILURE;
+  }
+
----------------
I think with `ExitOnError` this could be:
```
std::unique_ptr<Binary> BinLHS = ExitOnErr(convertFileToBinary(InputFileNameLHS));
```

It is not uncommon to have it as a static global in a tool's driver cpp and use it directly all over the place. There is a number of good examples, e.g. llvm-link https://github.com/llvm/llvm-project/blob/81bc732816107f6aff4fd61980f7b03cc92332b5/llvm/tools/llvm-link/llvm-link.cpp#L296


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101835