[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:101
+
+  void printTree();
+
----------------
Should this function be `const`?


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:112
+    TreeNode &addChild(ArrayRef<UTF16> NameRef);
+    void print(ScopedPrinter &Writer, std::string Name);
+
----------------
Can this argument be a `StringRef`?


================
Comment at: llvm/lib/Object/WindowsResource.cpp:80
   RETURN_IF_ERROR(Reader.readInteger(HeaderSize));
+
+  if (HeaderSize < MIN_HEADER_SIZE)
----------------
I think this function would be a lot clearer if we had some structs that we could just read out in one or two lines rather than reading some fields, ignoring others, skipping sometimes, reading sometimes, etc.  How about this?

```
class ResourceEntryRef {
  struct HeaderPrefix {
    ulittle32_t DataSize;
    ulittle32_t HeaderSize;
    ulittle16_t Unknown;
    ulittle16_t TypeID;
    ulittle16_t IDFlag;
    ulittle16_t NameID;
  };

  struct HeaderSuffix {
    ulittle32_t DataVersion;
    ulittle16_t MemoryFlags;
    ulittle16_t LanguageID;
    ulittle32_t Version;
    ulittle32_t Characteristics;
  };

  const HeaderPrefix *Prefix = nullptr;
  const HeaderSuffix *Suffix = nullptr;
};

...

Error ResourceEntryRef::loadNext() {
  RETURN_IF_ERROR(Reader.readObject(Prefix));
  if (Prefix->IDFlag != 0xFFFF) {
    Reader.setOffset(Reader.getOffset() - 4);
    RETURN_IF_ERROR(Reader.readWideString(Name));
  }

  RETURN_IF_ERROR(Reader.readObject(Suffix));
  RETURN_IF_ERROR(Reader.readArray(Data, Prefix->DataSize);
  RETURN_IF_ERROR(Reader.padToAlignment(sizeof(uint32_t));
  return Error::success();
}
```


================
Comment at: llvm/lib/Object/WindowsResource.cpp:100
+        sizeof(uint16_t)); // Re-read the bytes which we used to check the flag.
+    uint32_t NameSize = HeaderSize - sizeof(uint32_t) /*Type ID field*/
+                        - sizeof(uint32_t)            /*DataVersion field*/
----------------
Is this math actually necessary?  It sounds like we just want to read until the first null terminator.  Maybe add a function to `BinaryStreamReader` similar to `readCString` but called `readWideString`?  


================
Comment at: llvm/lib/Object/WindowsResource.cpp:206-209
+    StringChildren.insert(
+        std::pair<std::string,
+                  std::unique_ptr<WindowsResourceParser::TreeNode>>(
+            NameString, std::move(NewChild)));
----------------
Can you try this?

```
StringChildren.emplace(NameString, std::move(NewChild));
```


https://reviews.llvm.org/D33566