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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:92
+      : GenericBinaryError(Msg, ECOverride) {}
+  static char ID;
+};
----------------
What is `ID`?


================
Comment at: llvm/lib/Object/WindowsResource.cpp:74
+  if (auto E = loadNext()) {
+    consumeError(std::move(Err));
+    Err = std::move(E);
----------------
I'm not sure if I understand this code correctly. Does this swallow Err if a new error occurs?

If you can change this part of code, a better approach to create an instance of some class whose constructor can fail is to provide a factory function and returns an Error or a new instance from the factory function.

For example, instead of providing a ctor that can fail, you could provide a static member function:

  static ErrorOr<ResourceEntryRef> ResourceEntryRef::create(BinaryStreamRef Ref,
    const WindowsResource *Owner);
 
which in turn call the ctor only when all conditions are met (thus the ctor is free from failure.)


================
Comment at: llvm/lib/Object/WindowsResource.cpp:137
+    if (E.isA<EmptyResError>()) {
+      consumeError(std::move(E));
+      return Error::success();
----------------
Does this ignore errors? Crash bug needs to be fixed, but I wonder if an empty file should be considered as a valid input.


https://reviews.llvm.org/D37044