[PATCH] D30908: [llvm-pdbdump] Add the beginning of PDB diffing support

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added inline comments.


================
Comment at: llvm/tools/llvm-pdbdump/Diff.cpp:166
+                                        QI.end(), Common.data(), Comparator);
+    OnlyP.set_size(std::distance(OnlyP.data(), PEnd));
+    OnlyQ.set_size(std::distance(OnlyQ.data(), QEnd));
----------------
zturner wrote:
> amccarth wrote:
> > In practice, I suspect `.data()` will work, but `.begin()` would seem more principled, since these algorithms work with pairs of iterators.  The fact that SmallVector iterators are pointers (as returned by `.data()`) is an implementation detail I wouldn't rely on.
> `begin()` would actually be wrong here.  I've only called `reserve`, not `resize`, so `size() == 0`.  `SmallVector<>` has this optimization so that you can copy objects into it without default constructing them first, and then calling `set_size()` (which is different than `resize()`) after you write into its internal buffer.
I understand you've only reserved space using this SmallVector feature.  I would expect begin on an empty SmallVector with a reservation to return an iterator at the beginning of the buffer and for end to return the same thing.

In fact, the implementation of `SmallVector::data()` is to return `begin()` coerced to the container's pointer type (which happens to be identical to its iterator type).

      /// Return a pointer to the vector's buffer, even if empty().
      pointer data() { return pointer(begin()); }

I normally wouldn't go diving into a container's implementation details, but it's the only place I can find that documents this feature of using the reserved buffer of an empty container.

Mixing pointers and iterators of a SmallVector works because they're interchangeable, but it seems to make this code fragile if someone were ever to come along and select a different container type, where the same effect could be achieved with a reserve and a back_insert_iterator.


https://reviews.llvm.org/D30908