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

[David Li via llvm-commits]

davidxl 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)
----------------
ikudrin wrote:
> 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.
> 
ack.   


http://reviews.llvm.org/D18610