[PATCH] D34020: Implement COFF emission for parsed Windows Resource ( .res) files.

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:127-128
+  const TreeNode &getTree() const { return Root; }
+  const std::vector<std::vector<uint8_t>> &getData() const { return Data; }
+  const std::vector<std::vector<UTF16>> &getStringTable() const { return StringTable; }
 
----------------
Maybe you can return `ArrayRef<std::vector<uint8_t>>` here?  Similarly for other function.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:137-143
+    Data.emplace_back(Entry.getData());
+
+    if (Entry.checkTypeString())
+      StringTable.emplace_back(Entry.getTypeString());
+
+    if (Entry.checkNameString())
+      StringTable.emplace_back(Entry.getNameString());
----------------
I don't think `emplace_back` is a win here, but it makes the code a little mroe confusing.  Can you change this to `push_back`?


================
Comment at: llvm/lib/Object/WindowsResource.cpp:164-165
 
-WindowsResourceParser::TreeNode::TreeNode(ArrayRef<UTF16> NameRef)
-    : Name(NameRef) {}
+WindowsResourceParser::TreeNode::TreeNode(bool IsStringNode)
+    : IsDataNode(false), MajorVersion(0), MinorVersion(0), Characteristics(0) {
+  if (IsStringNode)
----------------
If it's not a string node and it's not a data node, then what is it?


================
Comment at: llvm/lib/Object/WindowsResource.cpp:165
+WindowsResourceParser::TreeNode::TreeNode(bool IsStringNode)
+    : IsDataNode(false), MajorVersion(0), MinorVersion(0), Characteristics(0) {
+  if (IsStringNode)
----------------
Can you just initialize these inline in the class definition so we don't need these here?


================
Comment at: llvm/lib/Object/WindowsResource.cpp:170-178
+WindowsResourceParser::TreeNode::TreeNode(bool IsDataNode,
+                                          uint16_t MajorVersion,
+                                          uint16_t MinorVersion,
+                                          uint32_t Characteristics)
+    : IsDataNode(IsDataNode), MajorVersion(MajorVersion),
+      MinorVersion(MinorVersion), Characteristics(Characteristics) {
+  if (IsDataNode)
----------------
What happens if someone passes false here?  Is it even valid to have a data node with a version and characteristics?

These two constructors are really confusing.  What do you think about making the constructors private and then adding static functions

```
std::unique_ptr<TreeNode> createDataNode(uint16_t Major, uint16_t Minor, uint32_t Characteristics);
```

etc?


================
Comment at: llvm/lib/Object/WindowsResource.cpp:311
+  uint32_t SectionTwoOffset;
+  const std::vector<std::vector<UTF16>> &StringTable;
+  std::vector<uint32_t> StringTableOffsets;
----------------
`ArrayRef`?


https://reviews.llvm.org/D34020