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

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
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 {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124804