[Lldb-commits] [lldb] [NFC][lldb] Pass StackFrame by a reference in DIL (PR #201437)

[via lldb-commits]

jimingham wrote:

This is slightly orthogonal but implicated in this patch...  

Why does DILEval have a variable that is a lldb::StackFrameSP, but it is called `m_exe_ctx_scope`?  ExecutionContextScope are an important thing in lldb; they are a mixin class for StackFrame but also Target, Process, Thread etc.  A StackFrameSP is NOT an ExecutionContextScope and an ExecutionContextScope is NOT required to have a StackFrame.  It is also not a shared pointer, which this variable is.

That misnaming made this patch much more difficult to understand that it should have been.

But then the patch assumes that the poorly named `m_exe_ctx_scope` is never an empty StackFrameSP - since you dereference it without ever checking that.  The Interpreter class that holds the ivar is initialized from a StackFrameSP and doesn't seem to check whether it in empty either.  Maybe that's asserted at a higher layer, but I couldn't see where that was done.  If you are asserting that the StackFrameSP ivar you are holding can't be empty you should document that on the ivar, and maybe explain how that's ensured?

It seems like this patch removes unchecked dereferencing in functions for unsafe dereferencing at call sites.  That reduces the number of places where you are potentially making this error, but until it's clear why you can't make that error it still gives me the willies...

It's also a tiny bit odd that you don't use StackFrameSP but spell it out when you define this as well.

https://github.com/llvm/llvm-project/pull/201437