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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:60-65
   Error moveNext(bool &End);
+  Error getType(uint16_t &Type) const;
+  Error checkNameString(bool &isString) const;
+  Error getNameString(ArrayRef<UTF16> &Name) const;
+  Error getNameID(uint16_t &Name) const;
+  Error getLanguage(uint16_t &Language) const;
----------------
I don't think we need to propagate these `Error` values out of the function.  The constructor should assert that `HeaderBytes` is a good size, and at that point none of the reads can fail and you can just return the values instead of taking them by reference.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:112
+  public:
+    TreeNode() : ID(-1), Name() {}
+    explicit TreeNode(uint32_t ID) : ID(ID), Name() {}
----------------
You can just say `= default;`


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:113
+    TreeNode() : ID(-1), Name() {}
+    explicit TreeNode(uint32_t ID) : ID(ID), Name() {}
+    explicit TreeNode(ArrayRef<UTF16> NameRef) : ID(-1), Name() {
----------------
No need to explicitly initialize `Name`, its default constructor will get invoked implicitly.  


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:114
+    explicit TreeNode(uint32_t ID) : ID(ID), Name() {}
+    explicit TreeNode(ArrayRef<UTF16> NameRef) : ID(-1), Name() {
+      Name.resize(NameRef.size());
----------------
Same as above, but this time for `ID`, and you can initialize `Name` directly I believe.   `Name(NameRef)`


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:179
+  private:
+    uint16_t ID;
+    std::vector<UTF16> Name;
----------------
Can you do an inline initialization here?  `uint16_t ID = 0xFFFF;`


================
Comment at: llvm/lib/Object/WindowsResource.cpp:89-91
+  bool IsString;
+  RETURN_IF_ERROR(checkNameString(IsString));
+  if (!IsString)
----------------
Instead of doing this seeking and reading on every function call, can you just initialize all these values in the constructor and then just return them?  It makes the logic simpler to follow if you just read everything out all at once, because then there's no seeking going on.  It's just read / read / read / read.


https://reviews.llvm.org/D33566