[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:
> ikudrin wrote:
> > 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?
> unique call returns a iterator pointing to the new logical end of the vector. The dups are from logical end to the original end. You are right that lower_bound O(log(N)), but the overall complexity is O(N*log(N)) only when the # of dups is linear to the total number of regions.  I doubt it is the case in real case, but your concern is legit.
According to http://en.cppreference.com/w/cpp/algorithm/unique:

> Iterators pointing to an element between the new logical end and the physical end of the range are still dereferenceable, but the elements themselves have unspecified values.

So, you can't expect the dupes to be stored there.



http://reviews.llvm.org/D18610