[Lldb-commits] [lldb] d657519 - [lldb] Improve maintainability and readability for ValueObject methods (#75865)
via lldb-commits
lldb-commits at lists.llvm.org
Tue Jan 23 16:07:57 PST 2024
Author: Pete Lawrence
Date: 2024-01-23T16:07:52-08:00
New Revision: d657519838e4b2310e13ec5ff52599e041860825
URL: https://github.com/llvm/llvm-project/commit/d657519838e4b2310e13ec5ff52599e041860825
DIFF: https://github.com/llvm/llvm-project/commit/d657519838e4b2310e13ec5ff52599e041860825.diff
LOG: [lldb] Improve maintainability and readability for ValueObject methods (#75865)
As I worked through changes to another PR
(https://github.com/llvm/llvm-project/pull/74912), I couldn't help but
rewrite a few methods for readability, maintainability, and possibly
some behavior correctness too.
1. Exiting early instead of nested `if`-statements, which:
- Reduces indentation levels for all subsequent lines
- Treats missing pre-conditions similar to an error
- Clearly indicates that the full length of the method is the "happy
path".
2. Explicitly return empty Value Object shared pointers for those error
(like) situations, which
- Reduces the time it takes a maintainer to figure out what the method
actually returns based on those conditions.
3. Converting a mix of `if` and `if`-`else`-statements around an enum
into one `switch` statement, which:
- Consolidates the former branching logic
- Lets the compiler warn you of a (future) missing enum case
- This one may actually change behavior slightly, because what was an
early test for one enum case, now happens later on in the `switch`.
4. Consolidating near-identical, "copy-pasta" logic into one place,
which:
- Separates the common code to the diverging paths.
- Highlights the differences between the code paths.
rdar://119833526
Added:
Modified:
lldb/source/Core/ValueObject.cpp
Removed:
################################################################################
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index 9208047be36668d..d58bf2ca763d9e2 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -1582,62 +1582,64 @@ bool ValueObject::IsUninitializedReference() {
ValueObjectSP ValueObject::GetSyntheticArrayMember(size_t index,
bool can_create) {
- ValueObjectSP synthetic_child_sp;
- if (IsPointerType() || IsArrayType()) {
- std::string index_str = llvm::formatv("[{0}]", index);
- ConstString index_const_str(index_str);
- // Check if we have already created a synthetic array member in this valid
- // object. If we have we will re-use it.
- synthetic_child_sp = GetSyntheticChild(index_const_str);
- if (!synthetic_child_sp) {
- ValueObject *synthetic_child;
- // We haven't made a synthetic array member for INDEX yet, so lets make
- // one and cache it for any future reference.
- synthetic_child = CreateChildAtIndex(0, true, index);
-
- // Cache the value if we got one back...
- if (synthetic_child) {
- AddSyntheticChild(index_const_str, synthetic_child);
- synthetic_child_sp = synthetic_child->GetSP();
- synthetic_child_sp->SetName(ConstString(index_str));
- synthetic_child_sp->m_flags.m_is_array_item_for_pointer = true;
- }
- }
- }
+ if (!IsPointerType() && !IsArrayType())
+ return ValueObjectSP();
+
+ std::string index_str = llvm::formatv("[{0}]", index);
+ ConstString index_const_str(index_str);
+ // Check if we have already created a synthetic array member in this valid
+ // object. If we have we will re-use it.
+ if (auto existing_synthetic_child = GetSyntheticChild(index_const_str))
+ return existing_synthetic_child;
+
+ // We haven't made a synthetic array member for INDEX yet, so lets make
+ // one and cache it for any future reference.
+ ValueObject *synthetic_child = CreateChildAtIndex(0, true, index);
+
+ if (!synthetic_child)
+ return ValueObjectSP();
+
+ // Cache the synthetic child's value because it's valid.
+ AddSyntheticChild(index_const_str, synthetic_child);
+ auto synthetic_child_sp = synthetic_child->GetSP();
+ synthetic_child_sp->SetName(ConstString(index_str));
+ synthetic_child_sp->m_flags.m_is_array_item_for_pointer = true;
return synthetic_child_sp;
}
ValueObjectSP ValueObject::GetSyntheticBitFieldChild(uint32_t from, uint32_t to,
bool can_create) {
- ValueObjectSP synthetic_child_sp;
- if (IsScalarType()) {
- std::string index_str = llvm::formatv("[{0}-{1}]", from, to);
- ConstString index_const_str(index_str);
- // Check if we have already created a synthetic array member in this valid
- // object. If we have we will re-use it.
- synthetic_child_sp = GetSyntheticChild(index_const_str);
- if (!synthetic_child_sp) {
- uint32_t bit_field_size = to - from + 1;
- uint32_t bit_field_offset = from;
- if (GetDataExtractor().GetByteOrder() == eByteOrderBig)
- bit_field_offset =
- GetByteSize().value_or(0) * 8 - bit_field_size - bit_field_offset;
- // We haven't made a synthetic array member for INDEX yet, so lets make
- // one and cache it for any future reference.
- ValueObjectChild *synthetic_child = new ValueObjectChild(
- *this, GetCompilerType(), index_const_str, GetByteSize().value_or(0),
- 0, bit_field_size, bit_field_offset, false, false,
- eAddressTypeInvalid, 0);
-
- // Cache the value if we got one back...
- if (synthetic_child) {
- AddSyntheticChild(index_const_str, synthetic_child);
- synthetic_child_sp = synthetic_child->GetSP();
- synthetic_child_sp->SetName(ConstString(index_str));
- synthetic_child_sp->m_flags.m_is_bitfield_for_scalar = true;
- }
- }
- }
+ if (!IsScalarType())
+ return ValueObjectSP();
+
+ std::string index_str = llvm::formatv("[{0}-{1}]", from, to);
+ ConstString index_const_str(index_str);
+
+ // Check if we have already created a synthetic array member in this valid
+ // object. If we have we will re-use it.
+ if (auto existing_synthetic_child = GetSyntheticChild(index_const_str))
+ return existing_synthetic_child;
+
+ uint32_t bit_field_size = to - from + 1;
+ uint32_t bit_field_offset = from;
+ if (GetDataExtractor().GetByteOrder() == eByteOrderBig)
+ bit_field_offset =
+ GetByteSize().value_or(0) * 8 - bit_field_size - bit_field_offset;
+
+ // We haven't made a synthetic array member for INDEX yet, so lets make
+ // one and cache it for any future reference.
+ ValueObjectChild *synthetic_child = new ValueObjectChild(
+ *this, GetCompilerType(), index_const_str, GetByteSize().value_or(0), 0,
+ bit_field_size, bit_field_offset, false, false, eAddressTypeInvalid, 0);
+
+ if (!synthetic_child)
+ return ValueObjectSP();
+
+ // Cache the synthetic child's value because it's valid.
+ AddSyntheticChild(index_const_str, synthetic_child);
+ auto synthetic_child_sp = synthetic_child->GetSP();
+ synthetic_child_sp->SetName(ConstString(index_str));
+ synthetic_child_sp->m_flags.m_is_bitfield_for_scalar = true;
return synthetic_child_sp;
}
@@ -1647,9 +1649,8 @@ ValueObjectSP ValueObject::GetSyntheticChildAtOffset(
ValueObjectSP synthetic_child_sp;
- if (name_const_str.IsEmpty()) {
+ if (name_const_str.IsEmpty())
name_const_str.SetString("@" + std::to_string(offset));
- }
// Check if we have already created a synthetic array member in this valid
// object. If we have we will re-use it.
@@ -1659,13 +1660,13 @@ ValueObjectSP ValueObject::GetSyntheticChildAtOffset(
return synthetic_child_sp;
if (!can_create)
- return {};
+ return ValueObjectSP();
ExecutionContext exe_ctx(GetExecutionContextRef());
std::optional<uint64_t> size =
type.GetByteSize(exe_ctx.GetBestExecutionContextScope());
if (!size)
- return {};
+ return ValueObjectSP();
ValueObjectChild *synthetic_child =
new ValueObjectChild(*this, type, name_const_str, *size, offset, 0, 0,
false, false, eAddressTypeInvalid, 0);
@@ -1699,7 +1700,7 @@ ValueObjectSP ValueObject::GetSyntheticBase(uint32_t offset,
return synthetic_child_sp;
if (!can_create)
- return {};
+ return ValueObjectSP();
const bool is_base_class = true;
@@ -1707,7 +1708,7 @@ ValueObjectSP ValueObject::GetSyntheticBase(uint32_t offset,
std::optional<uint64_t> size =
type.GetByteSize(exe_ctx.GetBestExecutionContextScope());
if (!size)
- return {};
+ return ValueObjectSP();
ValueObjectChild *synthetic_child =
new ValueObjectChild(*this, type, name_const_str, *size, offset, 0, 0,
is_base_class, false, eAddressTypeInvalid, 0);
@@ -1736,30 +1737,30 @@ static const char *SkipLeadingExpressionPathSeparators(const char *expression) {
ValueObjectSP
ValueObject::GetSyntheticExpressionPathChild(const char *expression,
bool can_create) {
- ValueObjectSP synthetic_child_sp;
ConstString name_const_string(expression);
// Check if we have already created a synthetic array member in this valid
// object. If we have we will re-use it.
- synthetic_child_sp = GetSyntheticChild(name_const_string);
- if (!synthetic_child_sp) {
- // We haven't made a synthetic array member for expression yet, so lets
- // make one and cache it for any future reference.
- synthetic_child_sp = GetValueForExpressionPath(
- expression, nullptr, nullptr,
- GetValueForExpressionPathOptions().SetSyntheticChildrenTraversal(
- GetValueForExpressionPathOptions::SyntheticChildrenTraversal::
- None));
-
- // Cache the value if we got one back...
- if (synthetic_child_sp.get()) {
- // FIXME: this causes a "real" child to end up with its name changed to
- // the contents of expression
- AddSyntheticChild(name_const_string, synthetic_child_sp.get());
- synthetic_child_sp->SetName(
- ConstString(SkipLeadingExpressionPathSeparators(expression)));
- }
- }
- return synthetic_child_sp;
+ if (auto existing_synthetic_child = GetSyntheticChild(name_const_string))
+ return existing_synthetic_child;
+
+ // We haven't made a synthetic array member for expression yet, so lets
+ // make one and cache it for any future reference.
+ auto path_options = GetValueForExpressionPathOptions();
+ path_options.SetSyntheticChildrenTraversal(
+ GetValueForExpressionPathOptions::SyntheticChildrenTraversal::None);
+ auto synthetic_child =
+ GetValueForExpressionPath(expression, nullptr, nullptr, path_options);
+
+ if (!synthetic_child)
+ return ValueObjectSP();
+
+ // Cache the synthetic child's value because it's valid.
+ // FIXME: this causes a "real" child to end up with its name changed to
+ // the contents of expression
+ AddSyntheticChild(name_const_string, synthetic_child.get());
+ synthetic_child->SetName(
+ ConstString(SkipLeadingExpressionPathSeparators(expression)));
+ return synthetic_child;
}
void ValueObject::CalculateSyntheticValue() {
@@ -1956,66 +1957,55 @@ ValueObjectSP ValueObject::GetValueForExpressionPath(
const GetValueForExpressionPathOptions &options,
ExpressionPathAftermath *final_task_on_target) {
- ExpressionPathScanEndReason dummy_reason_to_stop =
- ValueObject::eExpressionPathScanEndReasonUnknown;
- ExpressionPathEndResultType dummy_final_value_type =
- ValueObject::eExpressionPathEndResultTypeInvalid;
- ExpressionPathAftermath dummy_final_task_on_target =
- ValueObject::eExpressionPathAftermathNothing;
-
- ValueObjectSP ret_val = GetValueForExpressionPath_Impl(
- expression, reason_to_stop ? reason_to_stop : &dummy_reason_to_stop,
- final_value_type ? final_value_type : &dummy_final_value_type, options,
- final_task_on_target ? final_task_on_target
- : &dummy_final_task_on_target);
-
- if (!final_task_on_target ||
- *final_task_on_target == ValueObject::eExpressionPathAftermathNothing)
- return ret_val;
-
- if (ret_val.get() &&
- ((final_value_type ? *final_value_type : dummy_final_value_type) ==
- eExpressionPathEndResultTypePlain)) // I can only deref and takeaddress
- // of plain objects
- {
- if ((final_task_on_target ? *final_task_on_target
- : dummy_final_task_on_target) ==
- ValueObject::eExpressionPathAftermathDereference) {
- Status error;
- ValueObjectSP final_value = ret_val->Dereference(error);
- if (error.Fail() || !final_value.get()) {
- if (reason_to_stop)
- *reason_to_stop =
- ValueObject::eExpressionPathScanEndReasonDereferencingFailed;
- if (final_value_type)
- *final_value_type = ValueObject::eExpressionPathEndResultTypeInvalid;
- return ValueObjectSP();
- } else {
- if (final_task_on_target)
- *final_task_on_target = ValueObject::eExpressionPathAftermathNothing;
- return final_value;
- }
- }
- if (*final_task_on_target ==
- ValueObject::eExpressionPathAftermathTakeAddress) {
- Status error;
- ValueObjectSP final_value = ret_val->AddressOf(error);
- if (error.Fail() || !final_value.get()) {
- if (reason_to_stop)
- *reason_to_stop =
- ValueObject::eExpressionPathScanEndReasonTakingAddressFailed;
- if (final_value_type)
- *final_value_type = ValueObject::eExpressionPathEndResultTypeInvalid;
- return ValueObjectSP();
- } else {
- if (final_task_on_target)
- *final_task_on_target = ValueObject::eExpressionPathAftermathNothing;
- return final_value;
- }
- }
+ auto dummy_stop_reason = eExpressionPathScanEndReasonUnknown;
+ auto dummy_value_type = eExpressionPathEndResultTypeInvalid;
+ auto dummy_final_task = eExpressionPathAftermathNothing;
+
+ auto proxy_stop_reason = reason_to_stop ? reason_to_stop : &dummy_stop_reason;
+ auto proxy_value_type =
+ final_value_type ? final_value_type : &dummy_value_type;
+ auto proxy_final_task =
+ final_task_on_target ? final_task_on_target : &dummy_final_task;
+
+ auto ret_value = GetValueForExpressionPath_Impl(expression, proxy_stop_reason,
+ proxy_value_type, options,
+ proxy_final_task);
+
+ // The caller knows nothing happened if `final_task_on_target` doesn't change.
+ if (!ret_value || (*proxy_value_type) != eExpressionPathEndResultTypePlain ||
+ !final_task_on_target)
+ return ValueObjectSP();
+
+ ExpressionPathAftermath &final_task_on_target_ref = (*final_task_on_target);
+ ExpressionPathScanEndReason stop_reason_for_error;
+ Status error;
+ // The method can only dereference and take the address of plain objects.
+ switch (final_task_on_target_ref) {
+ case eExpressionPathAftermathNothing:
+ return ret_value;
+
+ case eExpressionPathAftermathDereference:
+ ret_value = ret_value->Dereference(error);
+ stop_reason_for_error = eExpressionPathScanEndReasonDereferencingFailed;
+ break;
+
+ case eExpressionPathAftermathTakeAddress:
+ ret_value = ret_value->AddressOf(error);
+ stop_reason_for_error = eExpressionPathScanEndReasonTakingAddressFailed;
+ break;
+ }
+
+ if (ret_value && error.Success()) {
+ final_task_on_target_ref = eExpressionPathAftermathNothing;
+ return ret_value;
}
- return ret_val; // final_task_on_target will still have its original value, so
- // you know I did not do it
+
+ if (reason_to_stop)
+ *reason_to_stop = stop_reason_for_error;
+
+ if (final_value_type)
+ *final_value_type = eExpressionPathEndResultTypeInvalid;
+ return ValueObjectSP();
}
ValueObjectSP ValueObject::GetValueForExpressionPath_Impl(
@@ -2686,39 +2676,47 @@ ValueObjectSP ValueObject::AddressOf(Status &error) {
const bool scalar_is_load_address = false;
addr_t addr = GetAddressOf(scalar_is_load_address, &address_type);
error.Clear();
- if (addr != LLDB_INVALID_ADDRESS && address_type != eAddressTypeHost) {
- switch (address_type) {
- case eAddressTypeInvalid: {
- StreamString expr_path_strm;
- GetExpressionPath(expr_path_strm);
- error.SetErrorStringWithFormat("'%s' is not in memory",
- expr_path_strm.GetData());
- } break;
- case eAddressTypeFile:
- case eAddressTypeLoad: {
- CompilerType compiler_type = GetCompilerType();
- if (compiler_type) {
- std::string name(1, '&');
- name.append(m_name.AsCString(""));
- ExecutionContext exe_ctx(GetExecutionContextRef());
- m_addr_of_valobj_sp = ValueObjectConstResult::Create(
- exe_ctx.GetBestExecutionContextScope(),
- compiler_type.GetPointerType(), ConstString(name.c_str()), addr,
- eAddressTypeInvalid, m_data.GetAddressByteSize());
- }
- } break;
- default:
- break;
- }
- } else {
- StreamString expr_path_strm;
- GetExpressionPath(expr_path_strm);
+ StreamString expr_path_strm;
+ GetExpressionPath(expr_path_strm);
+ const char *expr_path_str = expr_path_strm.GetData();
+
+ ExecutionContext exe_ctx(GetExecutionContextRef());
+ auto scope = exe_ctx.GetBestExecutionContextScope();
+
+ if (addr == LLDB_INVALID_ADDRESS) {
error.SetErrorStringWithFormat("'%s' doesn't have a valid address",
- expr_path_strm.GetData());
+ expr_path_str);
+ return ValueObjectSP();
}
- return m_addr_of_valobj_sp;
+ switch (address_type) {
+ case eAddressTypeInvalid:
+ error.SetErrorStringWithFormat("'%s' is not in memory", expr_path_str);
+ return ValueObjectSP();
+
+ case eAddressTypeHost:
+ error.SetErrorStringWithFormat("'%s' is in host process (LLDB) memory",
+ expr_path_str);
+ return ValueObjectSP();
+
+ case eAddressTypeFile:
+ case eAddressTypeLoad: {
+ CompilerType compiler_type = GetCompilerType();
+ if (!compiler_type) {
+ error.SetErrorStringWithFormat("'%s' doesn't have a compiler type",
+ expr_path_str);
+ return ValueObjectSP();
+ }
+
+ std::string name(1, '&');
+ name.append(m_name.AsCString(""));
+ m_addr_of_valobj_sp = ValueObjectConstResult::Create(
+ scope, compiler_type.GetPointerType(), ConstString(name.c_str()), addr,
+ eAddressTypeInvalid, m_data.GetAddressByteSize());
+ return m_addr_of_valobj_sp;
+ }
+ }
}
ValueObjectSP ValueObject::DoCast(const CompilerType &compiler_type) {
More information about the lldb-commits
mailing list