[PATCH] D37044: Fix bug 34051 by handling empty .res files gracefully.

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:92
+      : GenericBinaryError(Msg, ECOverride) {}
+  static char ID;
+};
----------------
ruiu wrote:
> What is `ID`?
Ah you're right it's not necessary.  Normally subclasses of ErrorInfo need to have this defined or else the linker won't be happy but I guess that only applies to direct non-virtual subclasses.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:137
+    if (E.isA<EmptyResError>()) {
+      consumeError(std::move(E));
+      return Error::success();
----------------
ruiu wrote:
> Does this ignore errors? Crash bug needs to be fixed, but I wonder if an empty file should be considered as a valid input.
Not an empty file, it still contains PE header magic and an empty, null entry.  So it's a valid .res file that just doesn't contain any records, which I could see being valid in some cases.  Also, the original cvtres does not crash/throw error on empty .res.


A completely empty file would fail upon WindowsResource::createWindowsResource().


https://reviews.llvm.org/D37044