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

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
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)
----------------
zturner wrote:
> If it's not a string node and it's not a data node, then what is it?
There are two types of nodes: those that point to data, and those that point to children nodes.  In the later case, the node can be identified by either a string or an integer id.  So if it's not a string node and not a data node, it's a non-data node identified by an integer id.

I should probably make this distinction more clear in the constructors.  I've changed it so that one constructor is always used for creating non-data nodes, an accepts a flag for making it a string node.  The other constructor makes only data nodes.


================
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)
----------------
zturner wrote:
> 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?
Hmmm intuitively it would seem that only data nodes should have version and characteristics, however I've had trouble verifying this and I haven't been able to generate a .res file where the version and characteristics are set to anything other than zero.  For now I think it's safe to assume that only nodes containing data would have a version and characteristic for that data specified.

I've tried to make things more clear in the constructors by having one used only for making data nodes and one for all others.


https://reviews.llvm.org/D34020