[PATCH] D33566: Adding parsing ability for .res file, building the resource directory tree

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:51-54
+#define RETURN_IF_ERROR(X)                                                     \
+  if (auto EC = X)                                                             \
+    return std::move(EC);
+}
----------------
ruiu wrote:
> I'd move this out of `namespace llvm` as namespaces have no effect on C macros.
I moved it to the source file since it is no longer necessary in header.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:62
+  Error getType(uint16_t &Type) const;
+  Error checkNameString(bool &isString) const;
+  Error getNameString(ArrayRef<UTF16> &Name) const;
----------------
ruiu wrote:
> isString -> IsString
returns bool instead of Error now


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:131
+    }
+    Expected<TreeNode &> addChild(uint32_t ID) {
+      auto Child = IDChildren.find(ID);
----------------
ruiu wrote:
> This is a question not only for this but for all the other functions you added in this patch.
> 
> Can you do error checking only when you are reading files?
> 
> Currently, you are doing error checking both when reading files and writing to files. However, as long as you verify that input files, and you don't generate malformed data in your code (if it does, it is a bug), you don't need to check for errors when you are writing.
> 
> The reason why I'm saying is because the code is not easy to read because that contains probably too much error checking snippets. Other file reading/writing tools don't have this much ErrorOr or Expected internally, so they can be reduced.
All the error checking is now done when the file is initially read.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:179
+  private:
+    uint16_t ID;
+    std::vector<UTF16> Name;
----------------
zturner wrote:
> Can you do an inline initialization here?  `uint16_t ID = 0xFFFF;`
What is the purpose of this initialization?  We only ever check the ID if it is in the IDChildren list, and before adding to this list we always give the ID that was written in the .res file.


https://reviews.llvm.org/D33566