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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
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,
----------------
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?


================
Comment at: lib/ProfileData/Coverage/CoverageMapping.cpp:370
+    // its predecessor's count.
+    if (FirstCompletedRegion &&
+        FirstCompletedRegion + 1 == ActiveRegions.size()) {
----------------
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).


https://reviews.llvm.org/D36813