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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:404
   // Current location within the file.
   uint64_t CurPtr = 0;
 
----------------
sfertile wrote:
> 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.
In `ELFObjectFile.cpp`, the ctor is a thin wrapper.
In `COFFObjectFile.cpp` and `MachOObjectFile.cpp`, they pass an non-const reference of `Error` variable to carry the parsing error.
It seems to me the ELF approach is slightly better as we can just use the return type instead of passing another variable around.

```
// COFF
  std::unique_ptr<COFFObjectFile> Ret(new COFFObjectFile(Object, EC));
  if (EC)
//// the object is created then destroyed if there is an error.
    return errorCodeToError(EC);

// ELF
  auto Ret = ELFObjectFile<ELFT>::create(Object);
/// if there is a parse error, nothing gets created
  if (Error E = Ret.takeError())
    return std::move(E);
  return make_unique<ELFObjectFile<ELFT>>(std::move(*Ret));
```

I hope this wouldn't be too drastic a change to make, but if it is, feel free to keep the current shape and make improvement (if any) in the future :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61532