[PATCH] D136702: [llvm-cov] Look up object files using debuginfod

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath marked an inline comment as done.
mysterymath added inline comments.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:421
+    llvm::sort(ProfileBinaryIDs, Compare);
+    std::unique(ProfileBinaryIDs.begin(), ProfileBinaryIDs.end(), Compare);
+
----------------
dblaikie wrote:
> mysterymath wrote:
> > dblaikie wrote:
> > > mysterymath wrote:
> > > > dblaikie wrote:
> > > > > mysterymath wrote:
> > > > > > jgorbe wrote:
> > > > > > > We're getting build errors here (and the similar call to `std::unique` on line 426) with `-Wunused-result`.
> > > > > > > 
> > > > > > > This looks like a bug: `std::unique` returns the new end of the range, and typically you need to call `container.erase(new_end, container.end())` after the call to `std::unique` to reduce the container size to match the new logical size. See https://en.cppreference.com/w/cpp/algorithm/unique.
> > > > > > Thanks; did a quick fix forward for this. This uncovered a mistake with Compare as well; std::unique requires a == comparator, while sort and set_difference a < comparator.
> > > > > Is there test coverage for these bugs/fixes?
> > > > Partially; the unused result lint caught the issue with std::unique; fixing that caused the original test cases to fail due to the comparator issue.
> > > Could you see about adding a test case that would've failed with the code as originally committed? (something that demonstrates the need for the unique call in the first place)
> > I took a closer look at the gulfem's preceding change (https://reviews.llvm.org/D135929), and it appears that the std::unique here is superfluous; there's no effective way to produce duplicate build IDs in ProfileBinaryIDs, and in that case, they're harmless in FoundBinaryIDs, since std::max(1-k, 0) is 0 for any k >= 1. The ProfileBinaryIDs sort is also superfluous.
> > 
> > That does mean there's intrinsically no way to test it, since it makes no observable difference. I've thus removed the uniquing logic completely.
> ah, excellent - the best code is no code. Could you provide a link here to the commit hash/etc that removed it, for the record?
That's https://reviews.llvm.org/rG0a51eeda1ef43d0f9a73ed2ac15f602262943ea1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136702/new/

https://reviews.llvm.org/D136702