For reference, the original code that Greg wrote in r112301 was
+ if (!m_disassembly.GetString().empty())
+ m_disassembly.GetString().swap (m_disassembly.GetString());
> On Nov 14, 2016, at 1:44 PM, Zachary Turner <ztur...@google.com> wrote:
>
> If the swap is correct, then wouldn't we also need to swap the variable list?
>
> On Mon, Nov 14, 2016 at 10:58 AM Jim Ingham <jing...@apple.com> wrote:
>
> > On Nov 13, 2016, at 4:48 PM, Zachary Turner via lldb-dev
> > <lldb-dev@lists.llvm.org> wrote:
> >
> > I was going through doing some routine StringRef changes and I ran across
> > this function:
> >
> > std::lock_guard<std::recursive_mutex> guard(m_mutex);
> > assert(GetStackID() ==
> > prev_frame.GetStackID()); // TODO: remove this after some testing
> > m_variable_list_sp = prev_frame.m_variable_list_sp;
> >
> > m_variable_list_value_objects.Swap(prev_frame.m_variable_list_value_objects);
> > if (!m_disassembly.GetString().empty()) {
> > m_disassembly.Clear();
> > m_disassembly.GetString().swap(m_disassembly.GetString());
> > }
> >
> > Either I'm crazy or that bolded line is a bug. Is it supposed to be
> > prev_frame.m_disassembly.GetString()?
> >
> > What would the implications of this bug be? i.e. how can we write a test
> > for this?
> >
> > Also, as a matter of curiosity, why is it swapping? That means it's
> > modifying the input frame, when it seems like it really should just be
> > modifying the current frame.
>
> What lldb does is store the stack frame list it calculated from a previous
> stop, and copy as much as is relevant into the new stack frame when it stops,
> which will then become the stack frame list that gets used. So this is a
> transfer of information from the older stop's stack frame to the new one.
> Thus the swap.
>
> To be clear, current here means "the stack frame we are calculating from this
> stop" and previous here means "the stack frame from the last stop". That's
> confusing because previous & next also get used for up and down the current
> stack frame list. That's why I always try to use "younger" and "older" for
> ordering in one stack (that and it makes the ordering unambiguous.)
>
> So while this is definitely a bug, this is just going to keep the frames in
> the newly calculated stack frame list from taking advantage of any
> disassembly that was done on frames from the previous stop. Since this will
> get created on demand if left empty, it should have no behavioral effect. To
> test this you would have to count the number of times you disassembled the
> code for a given frame. If this were working properly, you'd only do it once
> for the time that frame lived on the stack. With this bug you will do it
> every time you stop and ask for disassembly for this frame.
>
> Jim
>
>
> > _______________________________________________
> > lldb-dev mailing list
> > lldb-dev@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
