[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:373
+      auto CompletedSegmentLoc = ActiveRegions[FirstCompletedRegion]->endLoc();
+      assert((!Loc || CompletedSegmentLoc <= Loc) &&
+             "Completed region ends after the designated location");
----------------
We can't get here with `Loc == None` because the only call where it is `None` sets `FirstCompletedRegion` to 0. So, the check for `Loc` in `assert` is a bit misleading. I'd add a separate `asset` to check that `Loc` is set.


================
Comment at: lib/ProfileData/Coverage/CoverageMapping.cpp:382
+    // the end location of the previous completed region.
+    for (unsigned I = FirstCompletedRegion + 1, E = ActiveRegions.size(); I < E;
+         ++I) {
----------------
The last of completed regions might not be processed completely, here is the test:
```
  ProfileWriter.addRecord({"func1", 0x1234, {1, 2, 3, 4}}, Err);
  startFunction("func1", 0x1234);

  addCMR(Counter::getCounter(0), "file1", 1, 1, 8, 8);
  addCMR(Counter::getCounter(1), "file1", 2, 2, 5, 5);
  addCMR(Counter::getCounter(2), "file1", 3, 3, 4, 4);
  addCMR(Counter::getCounter(3), "file1", 6, 6, 7, 7);

  EXPECT_THAT_ERROR(loadCoverageMapping(), Succeeded());
  const auto FunctionRecords = LoadedCoverage->getCoveredFunctions();
  const auto &FunctionRecord = *FunctionRecords.begin();
  CoverageData Data = LoadedCoverage->getCoverageForFunction(FunctionRecord);
  std::vector<CoverageSegment> Segments(Data.begin(), Data.end());

  ASSERT_EQ(8U, Segments.size());
  EXPECT_EQ(CoverageSegment(1, 1, 1, true), Segments[0]);
  EXPECT_EQ(CoverageSegment(2, 2, 2, true), Segments[1]);
  EXPECT_EQ(CoverageSegment(3, 3, 3, true), Segments[2]);
  EXPECT_EQ(CoverageSegment(4, 4, 2, false), Segments[3]);
  EXPECT_EQ(CoverageSegment(5, 5, 1, false), Segments[4]);
  EXPECT_EQ(CoverageSegment(6, 6, 4, true), Segments[5]);
  EXPECT_EQ(CoverageSegment(7, 7, 1, false), Segments[6]);
  EXPECT_EQ(CoverageSegment(8, 8, false), Segments[7]);
```


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


================
Comment at: lib/ProfileData/Coverage/CoverageMapping.cpp:436
+        startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
+                     CurStartLoc, true, (CR.index() + 1) == Regions.size());
+        continue;
----------------
I'd prefer seeing these expressions as separate statements, like

```
// If it's the last region, emit a skipped segment.
const bool Skipped = (CR.index() + 1) == Regions.size();
startSegment(..., Skipped);
```
but it's for you to decide.


https://reviews.llvm.org/D36813