[Lldb-commits] [lldb] r129595 - /lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.cpp

[Johnny Chen]

Hi Stephen,

I forgot to mention that ToT changed the 'merge' logic a little bit.
Instead of (r1, r2) merged into r1 (and drop r2), it was changed
to (r1, r2) merged into r2 (and drop r1).

The reason is that for r1, r2, r3 ... which might get merged into rn,
dropping r1 and updating r2 to the larger range works out better.

Thanks.

On Apr 15, 2011, at 1:51 PM, Stephen Wilson wrote:

> 
> Hi Johnny,
> 
> On Fri, Apr 15, 2011 at 07:56:24PM -0000, Johnny Chen wrote:
>> +    // Remove the merged ranges by shifting down all the keepers...
>> +    std::set<size_t> purged(indices.begin(), indices.end());
>> +    size_t new_size = m_aranges.size() - indices.size();
>> +    for (size_t src = 0, dst = 0; dst < new_size; ++dst)
>> +    {
>> +        while (purged.count(src) > 0)
>> +            ++src;
>> +        if (src == dst)
>> +            continue;
>> +        m_aranges[dst] = m_aranges[src];
>> +    }
> 
> That looks broken to me.  Index zero is never in the purged set since we
> merge ranges into their predecessors.  Therefore purged.count(0) == 0
> and on the first iteration we have src == dst, so we increment dst.
> What ends up happening is that src remains zero and we just do:
> 
>    for (i = 0; i < size; ++i) { m_arages[i] = m_aranges[0]; }
> 
> 
> Now, I *think* I know what you were trying to do here, and I think the
> main motivation on your side was to "clean up the code" -- but since the
> name of the game of the patch was optimization replacing the original
> O(n) approach with a "clean"  O(n log(n)) implementation is not the way
> to go IMHO.
> 
> Is there something I can do to clean up the original patch without
> reducing it's performance?
> 
> 
> Cheers,
> 
> -- 
> steve
>