[Lldb-commits] [PATCH] D74759: Treat RangeDataVector as an augmented BST

Fri Feb 28 00:32:55 PST 2020

labath added a comment.

In D74759#1896100 <https://reviews.llvm.org/D74759#1896100>, @unnar wrote:

> In D74759#1895748 <https://reviews.llvm.org/D74759#1895748>, @labath wrote:
>
> > That is a fair point. I suppose the reason why I see this as different is because this field is an implementation detail of the RangeDataVector class, and so the user should not even see it -- whereas the user has a legitimate reason to at least access the other fields (and most of the methods only provide read-only access to these fields).
> >
> > I'm sorry, I haven't gotten around to looking at this patch today, but I thought I'd at least say that...
>
>
> That is true. I am fine with changing it if that's the only thing that you see as blocking this change from passing.

I finally took a good look at the patch, and I think that is the only remaining question on my mind. Could you try implementing that to see how it looks like?

@teemperor, do you have any more thoughts on this?

================
Comment at: lldb/include/lldb/Utility/RangeMap.h:814
+    size_t mid = (lo + hi) / 2;
+    auto &entry = m_entries[mid];
+
----------------
[[ http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | Llvm style guide ]] does not recommend using `auto` in this situation. `Entry &entry` is one character longer, but it makes it clear what is going on.

================
Comment at: lldb/include/lldb/Utility/RangeMap.h:834
+    size_t mid = (lo + hi) / 2;
+    auto entry = m_entries[mid];
+
----------------
`const Entry &` -- http://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74759/new/

https://reviews.llvm.org/D74759