[PATCH] D18610: [Coverage] Restore the correct count value after processing a nested region in case of combined regions.

[Igor Kudrin via llvm-commits]

ikudrin added inline comments.

================
Comment at: llvm/trunk/lib/ProfileData/CoverageMapping.cpp:397
@@ +396,3 @@
+/// Combine counts of regions which cover the same area.
+template <class It> static It combineRegionCounts(It First, It Last) {
+  if (First == Last)
----------------
davidxl wrote:
> I think this can be simplified by calling  std::unique
> 
> Followed by a scan of the duplicate entries. For each dup entry, call lower_bound to to find the matching entry and update it if it exists.
I'm afraid I can't fully understand your idea. `std::unique` just removes duplicate entries and provides no way to work with removed items. So, I can't catch, how you suppose to scan dupes. Moreover, `std::lower_bound` requires O(log(N)) comparisons which leads to O(N*log(N)) complexity for the whole method. At the same time, the current code requires only one scan of an array and doesn't require additional dynamic memory.
Could you provide some code to illustrate your approach?

================
Comment at: llvm/trunk/lib/ProfileData/CoverageMapping.cpp:487
@@ -469,1 +486,3 @@
   sortNestedRegions(Regions.begin(), Regions.end());
+  Regions.erase(combineRegionCounts(Regions.begin(), Regions.end()),
+                Regions.end());
----------------
davidxl wrote:
> This probably can be moved into 'sortNestedRegions'.
As they are separate and mostly independent tasks, I don't think it's good to merge them in one function. On the other hand, we can move both `sortNestedRegions` and `combineRegionCounts` into the `SegmentBuilder` class and simplify callers this way.


http://reviews.llvm.org/D18610