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 <br><div class="gmail_quote"><div dir="ltr">On Fri, Jul 28, 2017 at 11:17 AM Adrian McCarthy via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">amccarth marked an inline comment as done.<br>
amccarth added inline comments.<br>
<br>
<br>
================<br>
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumTypes.cpp:27-29<br>
+  // This is expensive and doesn't really return what we want.  Do we need<br>
+  // this functionality?<br>
+  return static_cast<uint32_t>(std::distance(Range.begin(), Range.end()));<br>
----------------<br>
zturner wrote:<br>
> 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.<br>
Yeah, the "doesn't really return what we want" in the comment was intended to capture that this isn't right.<br>
<br>
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.<br>
<br>
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.<br>
<br>
<br>
<br>
<br>
================<br>
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumTypes.cpp:34<br>
+NativeEnumTypes::getChildAtIndex(uint32_t Index) const {<br>
+  // Not supported.  Random access is slow and currently unimportant.<br>
+  return nullptr;<br>
----------------<br>
zturner wrote:<br>
> Not crazy about leaving this unsupported, but if we're going to do it, at the very least it needs a `llvm_unreachable`.<br>
This is related to the previous comment.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D35738" rel="noreferrer" target="_blank">https://reviews.llvm.org/D35738</a><br>
<br>
<br>
<br>
</blockquote></div>