[PATCH] D124804: [Object][DX] Parse DXContainer Parts

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar accepted this revision.
kuhar added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/include/llvm/Object/DXContainer.h:39-42
+  // The PartIterator is a wrapper around the iterator for the PartOffsets
+  // member of the DXContainer. It contains a refernce to the container, and the
+  // current iterator value, as well as storage for a parsed part header.
+  class PartIterator {
----------------
beanz wrote:
> kuhar wrote:
> > kuhar wrote:
> > > nit: could we pull this class out of `DXContainer`? I think this would make it a bit more readable, especially since we have another level of nesting with the `PartData` struct.
> > Have you considered this suggestion?
> Sorry, I had a comment on this that I failed to submit.
> 
> The downside to pulling the iterator out of the class is that trivial methods end up needing to be implemented in the implementation files. For example, if you put the iterator first in the header, the iterator's constructor and updateIterator method need to be defined in the implementation file because they depend on the DXContainer class. Conversely if you put the iterator after DXContainer, DXContainer begin and end need to be in the implementation file.
> 
> Moving trivial functions to the implementation file prevents them from being inlined.
> 
> I also experimented with replacing PartData with a std::pair, but found the first/second accessors to be less intuitive to read than the named members.
You could define functions in the header if you are worried about performance in non-LTO builds. However, if you think that this change does not simplify take code or improve readability, I think that leaving it as-is is also fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124804