[PATCH] D36780: [llvm-dlltool] Don't crash if no def file is provided or it can't be opened

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:67
+  if (std::error_code EC = MB.getError()) {
     llvm::errs() << "fail openFile: " << EC.message() << "\n";
+    return None;
----------------
ruiu wrote:
> `openFile` is a function name and shouldn't be reported as part of an error message. It should be something like `"cannot open file " << Path << ": " << EC.message() << "\n"`.
> 
> But this is not new code, so we don't have to address it in this patch.
Ok - I can change that in a separate patch.


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:120-128
+  Optional<MemoryBufferRef> MB;
+  if (auto *Arg = Args.getLastArg(OPT_d)) {
     MB = openFile(Arg->getValue());
+    if (!MB)
+      return 1;
+  } else {
+    llvm::errs() << "no definition file specified\n";
----------------
ruiu wrote:
> I feel this way is more readable.
> 
>   if (!Args.hasArg(OPT_d))
>     llvm::errs() << "no definition file specified\n";
>     return 1;
>   }
> 
>   Optional<MemoryBufferRef> MB = openFile(Args.getLastArg(OPT_d)->getValue());
>   if (!MB)
>     return 1;
> 
Indeed, that's much more readable. Will commit in that form.


https://reviews.llvm.org/D36780