[PATCH] D61532: implement of the parsing symbol table for xcoffobjfile and output as yaml format

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:404
   // Current location within the file.
   uint64_t CurPtr = 0;
 
----------------
MaskRay wrote:
> The heavylifting parsing work should be done in `createXCOFFObjectFile`. The constructor should be private and be a very thin wrapper.
> The constructor should be private 

This makes sense to me.

> The heavylifting parsing work should be done in `createXCOFFObjectFile`.  ... be a very thin wrapper.

This seems to go against what most of the other file formats do. I think at this point it seems reasonable to move the parsing  we have into  `createXCOFFObjectFile` but we have a downstream patch under review locally for adding 64-bit support and  I believe such a change would seriously complicate the parsing/error handling needed to support the different parts of the object format. If the parsing fails we propagate the error to the caller, so I'm not sure if the state of the object is that big if a concern. I'd like to know what benefits your looking to get by moving the parsing into the create function, maybe its worth the extra complication or maybe we can capture parts of the benefit without too drastic a change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61532/new/

https://reviews.llvm.org/D61532