[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:117
+  if (opts::diff::Pedantic) {
+    size_t Min = std::min(P.size(), Q.size());
+    for (size_t I = 0; I < Min; ++I) {
----------------
Should you report if `P.size() != Q.size()`?


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


================
Comment at: llvm/tools/llvm-pdbdump/Diff.cpp:150
+    };
+    std::sort(PI.begin(), PI.end(), Comparator);
+    std::sort(QI.begin(), QI.end(), Comparator);
----------------
Is the Comparator necessary?  Isn't the default comparator std::less, which should do exactly the same thing?


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


================
Comment at: llvm/tools/llvm-pdbdump/StreamUtil.h:20
+class PDBFile;
+void discoverStreamPurposes(PDBFile &File,
+                            SmallVectorImpl<std::string> &Purposes);
----------------
Could you add a comment clarifying what this method does?


https://reviews.llvm.org/D30908