labath added a comment. In D130796#3692243 <https://reviews.llvm.org/D130796#3692243>, @zequanwu wrote:
> In D130796#3691227 <https://reviews.llvm.org/D130796#3691227>, @zequanwu > wrote: > >>> If you find yourself needing to do extra work to work its limitations, we >>> should fix that algorithm instead. >> >> That makes sense. I'll work on fixing `RangeVectorData`. > > After thinking it again, not just `RangeVectorData` need to be written but > also `RangeVector`, although they are similar. It requires rewrite the whole > file `RangeMap.h` and all other places uses its APIs. Why is that? I don't think we need to change the API -- just the way it is implemented. And the RangeVectorData class already contains the upper_bound argument, which should make it possible to find the element containing a value quickly <https://en.wikipedia.org/wiki/Interval_tree#Java_example:_Searching_a_point_or_an_interval_in_the_tree>. That basically means taking the algorithm in FindEntryIndexesThatContain, but making it stop at the first element found. And you can just ignore the RangeVector class. That problem (two separate classes) can be handled separately and holistically. Alternatively, if we don't want to/need to support overlaps, we could switch to some completely different class, which implements searches correctly. llvm::IntervalMap, for example. > Can we just use `RangeVectorData` as it is right now and fix it later? So, > this patch is not blocked. Otherwise, this local variables functionality is > almost unusable for NativePDB plugin when multiple ranges exist. Maybe.. However I don't want to end up with two broken overlap-detection algorithms (see inline comment) instead of one good one. ================ Comment at: lldb/source/Expression/DWARFExpressionList.cpp:37-39 + if (m_exprs.FindEntryThatContains(base) || + m_exprs.FindEntryThatContains(end - 1)) + return false; ---------------- zequanwu wrote: > labath wrote: > > What is this attempting to check? That the list does not contain an > > overlapping entry? That hardly seems like a correct algorithm... > `RangeDataVector::Append` just append the new entry to the end of vector. It > has no idea if overlaps happens or not. I get that. My point is that this is not the right way to check for overlaps. It won't catch `(1,10)` and `(4,6)` for instance (depending on the order in which you insert them). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130796/new/ https://reviews.llvm.org/D130796 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
