[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: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:
> 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);
> ```
Okay I've added these functions to clarify when each type of node is made.

In fact, nothing outside of the class should be able to create new nodes.  This is because creating new nodes relies on being able to keep the data and string table indexes in sync with the actual data and strings in the order they are read.  I've made all non-const functions private and added WindowsResourceParser as a friend class, since really TreeNode is a class it uses internally and they know how each other function.


https://reviews.llvm.org/D34020