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

[Johnny Chen]

Hi Stephen,

You're more than welcome to commit your patch after some testing.
Yes, my motivation was to clean up the code to make it more understandable.
But since it broke the logic, it's no good.

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
>