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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/tools/llvm-pdbdump/Diff.cpp:117
+  if (opts::diff::Pedantic) {
+    size_t Min = std::min(P.size(), Q.size());
+    for (size_t I = 0; I < Min; ++I) {
----------------
amccarth wrote:
> Should you report if `P.size() != Q.size()`?
That should get reported indirectly (because we will just report that one file has streams not present in the other file).


================
Comment at: llvm/tools/llvm-pdbdump/Diff.cpp:122
+                          File2.getStreamByteSize(I)};
+      bool NamesDiffer = Names[0] == Names[1];
+      bool SizesDiffer = Sizes[0] == Sizes[1];
----------------
amccarth wrote:
> Should that be `!=`?  (Next line as well.)
Yes, fixed in latest diff.


================
Comment at: llvm/tools/llvm-pdbdump/Diff.cpp:150
+    };
+    std::sort(PI.begin(), PI.end(), Comparator);
+    std::sort(QI.begin(), QI.end(), Comparator);
----------------
amccarth wrote:
> Is the Comparator necessary?  Isn't the default comparator std::less, which should do exactly the same thing?
`PI` is the value type of the enumerator range, which in this case is something like `result_pair<SmallString<32>>`.  Even if that weren't the case and it were just `std::pair<size_t, std::string>` though, this comparator is specifically designed to only look at the second item of the pair (the name) and not the index.


================
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));
----------------
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.


https://reviews.llvm.org/D30908