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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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);
+}
----------------
I'd move this out of `namespace llvm` as namespaces have no effect on C macros.


================
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;
----------------
isString -> IsString


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:110
+private:
+  class TreeNode {
+  public:
----------------
Move function definitions for this class to the .cpp file.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:117-118
+      std::copy(NameRef.begin(), NameRef.end(), Name.begin());
+    }
+    Expected<TreeNode &> addNameNode(const ResourceEntryRef &Entry) {
+      bool IsStringName;
----------------
(You want to add newlines between multi-line inline functions so that they don't look like a single blob of some code.)


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:131
+    }
+    Expected<TreeNode &> addChild(uint32_t ID) {
+      auto Child = IDChildren.find(ID);
----------------
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.


https://reviews.llvm.org/D33566