[PATCH] D35039: [llvm-pdbutil] Improve output of llvm-pdbutil diff mode

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/tools/llvm-pdbutil/DiffPrinter.h:43
+    DiffResult operator()(const T &Left, const U &Right) const {
+      return (Left == Right) ? DiffResult::IDENTICAL : DiffResult::EQUIVALENT;
+    }
----------------
rnk wrote:
> I don't quite get this. Basically we have a comparator which says "if they're different, no big deal, they're actually equivalent". Is that more or less the idea? Maybe instead of asking the caller to pass a comparator we should have a different printEquivalent method? It seems shorter and less magical.
Yea.  Like, In pdb 1 the stream for module foo has stream index 12, but in pdb 2 it has stream index 15.  These are equivalent because //a stream exists in both pdbs//.  The number doesn't matter.  If both of them had `kInvalidStreamIndex`, they would be identical.  If one had `kInvalidStreamIndex` and another had a valid stream index, these would be different.

I think we need some notion of a comparator because sometimes the comparisons get rather obtuse, and we want to wrap that up in an abstraction that can be re-used over and over.  For example, the stream number comparison described above.  But it even gets worse.  If it's a name index, then we actually have to go look in the PDB file, get the corresponding string out, and compare the strings themselves rather than the numbers.  

I have some enhancements to this model in a forthcoming patch (not up for review yet, but depends on all 3 of these changes) where the class has both a `format` method and a `compare` method.  Consider the case where you have two module names `foo.obj` and `bar.obj`.  You want to print `Module "foo.obj"` and `Module "bar.obj"` but you only want to compare the actual file paths, but you can't compare the strings directly because for really long paths you *also* need to truncate part of the module name so that it displays like `Module "...o.obj"` rather than `...dule "foo.obj"`.  So yea, it's kinda gross.

Note that the caller doesn't have to pass anything, it gets a default template argument of `Identical`.  In this patch, the caller does have to instantiate a default-constructed instance and pass that through, but in the forthcoming patch I mentioned, all you have to do is write:

`print<Equivalent>("Foo", A, B)`;

or

`print<CustomComparator>("Foo", A, B);`

and it just works.


https://reviews.llvm.org/D35039