[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/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)
----------------
ecbeckmann wrote:
> 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.
But now the constructor doesn't really express its intent clearly.  A user can't tell from looking that that the 3 argument version is for data nodes etc.  How about making the constructor private and having:

```
static std::unique_ptr<TreeNode> creteStringNode(StringRef Str);
static std::unique_ptr<TreeNode> creteStringNode(uint32_t StringId);
static std::unique_ptr<TreeNode> createDataNode(uint16_t Major, uint16_t Minor, uint32_t Characteristics);
```


https://reviews.llvm.org/D34020