[PATCH] D36813: [Coverage] Build sorted and unique segments

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked 4 inline comments as done.
vsk added a comment.

Thanks for your comments, I'll update the patch shortly.



================
Comment at: lib/ProfileData/Coverage/CoverageMapping.cpp:432
+        // as this one.
+      } else if (CurStartLoc == CR.value().endLoc()) {
+        // Avoid making this zero-length region active. If it's the last region,
----------------
ikudrin wrote:
> vsk wrote:
> > ikudrin wrote:
> > > There can't be another region after a zero-length region with the same StartLoc, right?
> > > So, if you check this condition first, it'll simplify the code because there will be no need for 'else' branches and an empty 'then' block.
> > I think there can. If I'm understanding you correctly, this should be handled by the "handle_consecutive_regions_with_zero_length" unit test.
> At this point, `Regions` are already sorted and combined. Thus:
>  * if `B` follows `A` and `A.startLoc() == B.startLoc()` then `A.endLoc() < B.endLoc()`.
>  * You fixed the malformed case in the other patch, so for any region `startLoc() <= endLoc()`.
>  * Consequently, if `B` follows `A` and `A.startLoc() == A.endLoc()`, then `B` can't have the same `startLoc()` as `A` because it violates one of previous statements.
> 
> As a result, the code might be simplified to something like this:
> ```
> if (CurStartLoc == CR.value.endLoc()) {
>   ...
>   continue;
> }
> if (CR.index + 1 == Regions.size() || CurStartLoc != Regions[CR.index() + 1].startLoc()
>   startSegment(CR.value(), CurStartLoc, true);
> 
> ActiveRegions.push_back(&CR.value());
> ```
> 
> What do you think?
That makes sense.


================
Comment at: lib/ProfileData/Coverage/CoverageMapping.cpp:370
+    // its predecessor's count.
+    if (FirstCompletedRegion &&
+        FirstCompletedRegion + 1 == ActiveRegions.size()) {
----------------
ikudrin wrote:
> I believe that this specific case, as long as the special handling of the last segment in the loop and adding a skipped segment in the end, all of them might be joined together. The function would be divided into the following simple steps:
> 
>   # std_stable_sort
>   # In the loop, process all completed regions, apart from the last.
>   # Remeber a pointer to the last region.
>   # Remove completed regions from the list.
>   # Decide if we need to add a segment started at `endLoc` of the last region (only if it differs from `Loc`)
>   # Add a skipped or regular segment depending on `ActiveRegions` (if it is empty or not).
Good point, I think this makes the change substantially simpler. I tested the updated patch in the same way (prepare a coverage report for clang with an asserts-enabled binary + unit/lit tests).


https://reviews.llvm.org/D36813