[PATCH] D35738: Enable llvm-pdbutil to list enumerations using native PDB reader

[Zachary Turner via llvm-commits]

I think if we're going to intentionally veer from DIA here and support less
functionality, we should remove any methods from the base interface that
only work in one implementation. The random access functionality can still
be available using a DIA specific iterator, but at least this way you
wouldn't even be able to call the broken method on a native enumerator
On Fri, Jul 28, 2017 at 11:17 AM Adrian McCarthy via Phabricator <
reviews at reviews.llvm.org> wrote:

> amccarth marked an inline comment as done.
> amccarth added inline comments.
>
>
> ================
> Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumTypes.cpp:27-29
> +  // This is expensive and doesn't really return what we want.  Do we need
> +  // this functionality?
> +  return static_cast<uint32_t>(std::distance(Range.begin(), Range.end()));
> ----------------
> zturner wrote:
> > This is incorrect.  `getChildCount()` is supposed to return the number
> of values that the enumerator is going to enumerate.  The value being
> returned here doesn't account for the fact that not everything is an enum.
> Also, I'm pretty sure we use this method in `llvm-pdbutil pretty`.  In any
> case, it seems pretty important to me.
> Yeah, the "doesn't really return what we want" in the comment was intended
> to capture that this isn't right.
>
> You and I had a discussion a while back about removing this functionality
> because it requires enumerating all the types an extra time just to count
> them, for rather minimal value.  But we didn't really reach a conclusion.
>
> As far as I can tell, pretty is the only user, and it seems like it adds
> only minimal value.  So my preference would be to remove the getChildCount
> method from the enumerators altogether.
>
>
>
>
> ================
> Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumTypes.cpp:34
> +NativeEnumTypes::getChildAtIndex(uint32_t Index) const {
> +  // Not supported.  Random access is slow and currently unimportant.
> +  return nullptr;
> ----------------
> zturner wrote:
> > Not crazy about leaving this unsupported, but if we're going to do it,
> at the very least it needs a `llvm_unreachable`.
> This is related to the previous comment.
>
> On the one hand, the interface defined in IPDBEnumChildren suggests it's a
> simple forward iterator (getNext, reset).  On the other hand, it's an
> indexed collection (getChildCount, getChildAtIndex).  It kind of violates
> the single responsibility principle.
>
> We could enumerate everything once, essentially building a container,
> which would let us implement all the methods efficiently at the cost of
> more memory and moving a bunch of the cost to object creation time.
>
> It isn't a big deal for a dumper that's going through everything once, but
> it's not hard to imagine other users that would prefer the slim iterator
> idea.
>
>
> https://reviews.llvm.org/D35738
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170728/54bc9dfe/attachment.html>