[lldb][nfc] Factor out code checking if Variable is in scope #143572
Conversation
This is useful for checking whether a variable is in scope inside a specific block.
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesThis is useful for checking whether a variable is in scope inside a specific block. Full diff: https://github.com/llvm/llvm-project/pull/143572.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Symbol/Variable.h b/lldb/include/lldb/Symbol/Variable.h
index c437624d1ea6d..b867369576a41 100644
--- a/lldb/include/lldb/Symbol/Variable.h
+++ b/lldb/include/lldb/Symbol/Variable.h
@@ -89,6 +89,9 @@ class Variable : public UserID, public std::enable_shared_from_this<Variable> {
bool IsInScope(StackFrame *frame);
+ /// Returns true if this variable is in scope at `addr` inside `block`.
+ bool IsInScope(Block &block, Address addr);
+
bool LocationIsValidForFrame(StackFrame *frame);
bool LocationIsValidForAddress(const Address &address);
diff --git a/lldb/source/Symbol/Variable.cpp b/lldb/source/Symbol/Variable.cpp
index 8244725aba545..0415758dd268a 100644
--- a/lldb/source/Symbol/Variable.cpp
+++ b/lldb/source/Symbol/Variable.cpp
@@ -290,28 +290,9 @@ bool Variable::IsInScope(StackFrame *frame) {
// this variable was defined in is currently
Block *deepest_frame_block =
frame->GetSymbolContext(eSymbolContextBlock).block;
- if (deepest_frame_block) {
- SymbolContext variable_sc;
- CalculateSymbolContext(&variable_sc);
-
- // Check for static or global variable defined at the compile unit
- // level that wasn't defined in a block
- if (variable_sc.block == nullptr)
- return true;
-
- // Check if the variable is valid in the current block
- if (variable_sc.block != deepest_frame_block &&
- !variable_sc.block->Contains(deepest_frame_block))
- return false;
-
- // If no scope range is specified then it means that the scope is the
- // same as the scope of the enclosing lexical block.
- if (m_scope_range.IsEmpty())
- return true;
-
- addr_t file_address = frame->GetFrameCodeAddress().GetFileAddress();
- return m_scope_range.FindEntryThatContains(file_address) != nullptr;
- }
+ Address frame_addr = frame->GetFrameCodeAddress();
+ if (deepest_frame_block)
+ return IsInScope(*deepest_frame_block, frame_addr);
}
break;
@@ -321,6 +302,27 @@ bool Variable::IsInScope(StackFrame *frame) {
return false;
}
+bool Variable::IsInScope(Block &block, Address addr) {
+ SymbolContext variable_sc;
+ CalculateSymbolContext(&variable_sc);
+
+ // Check for static or global variable defined at the compile unit
+ // level that wasn't defined in a block
+ if (variable_sc.block == nullptr)
+ return true;
+
+ // Check if the variable is valid in the current block
+ if (variable_sc.block != &block && !variable_sc.block->Contains(&block))
+ return false;
+
+ // If no scope range is specified then it means that the scope is the
+ // same as the scope of the enclosing lexical block.
+ if (m_scope_range.IsEmpty())
+ return true;
+
+ return m_scope_range.FindEntryThatContains(addr.GetFileAddress()) != nullptr;
+}
+
Status Variable::GetValuesForVariableExpressionPath(
llvm::StringRef variable_expr_path, ExecutionContextScope *scope,
GetVariableCallback callback, void *baton, VariableList &variable_list,
|
lldb/source/Symbol/Variable.cpp
Outdated
| @@ -321,6 +302,27 @@ bool Variable::IsInScope(StackFrame *frame) { | |||
| return false; | |||
| } | |||
|
|
|||
| bool Variable::IsInScope(Block &block, Address addr) { | |||
There was a problem hiding this comment.
Any reason to not pass this by const ref?
There was a problem hiding this comment.
Just an oopsie on my part!
There was a problem hiding this comment.
That one is just a pointer + integer, my understanding is that it is supposed to be passed by value?
There was a problem hiding this comment.
Mmmm it is a weak pointer + integer. How cheap is it to copy a weak pointer?
There was a problem hiding this comment.
Like std::shared_ptr, a typical implementation of weak_ptr stores two pointers:
a pointer to the control block; and
the stored pointer of the shared_ptr it was constructed from.
Seems like it is cheap?
There was a problem hiding this comment.
A quick grep shows 223 instances of it being passed by const ref and the class itself seems to be taking other Addresses by const-ref too. There are definitely a bunch of instances of it being passed by value, but I would say "it's only slightly more expensive" isn't a strong argument unless there's a benefit to weigh that against such as needing a copy to modify or something. My final argument would be that if we ever do address spaces, the class will grow, which makes the case for the reference more compelling.
There was a problem hiding this comment.
That's an interesting, given the current usage it makes sense to switch this to const reference.
Future changes aside, it's still worth pointing out that "by value" by default is usually the correct choice. An API that prescribes passing something by reference is also saying "it's a bad idea to copy this object", a warning to users of the API. Here, there is no such warning, so it is an API that makes a developer pause for nothing -- the warnings are usually "it's expensive", "you can't copy because of ownership semantics".
…3572) This is useful for checking whether a variable is in scope inside a specific block.
…3572) This is useful for checking whether a variable is in scope inside a specific block.
…3572) This is useful for checking whether a variable is in scope inside a specific block.
This is useful for checking whether a variable is in scope inside a specific block.