[Lldb-commits] [lldb] [lldb] Improve maintainability and readability (and behavior?)… (PR #75865)
Pete Lawrence via lldb-commits
lldb-commits at lists.llvm.org
Mon Dec 18 14:39:57 PST 2023
https://github.com/PortalPete created https://github.com/llvm/llvm-project/pull/75865
… by exiting early and consolidating code paths.
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
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
>From b2fe1adeb707160ba2a7b7f87d43d18cfdf29039 Mon Sep 17 00:00:00 2001
From: Pete Lawrence <plawrence at apple.com>
Date: Thu, 7 Dec 2023 12:14:01 -1000
Subject: [PATCH] [lldb] Improve maintainability and readability (and
behavior?) by exiting early and consolidating code paths
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
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
---
lldb/source/Core/ValueObject.cpp | 344 ++++++++++++++++---------------
1 file changed, 178 insertions(+), 166 deletions(-)
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index b13bffa0ca809b..7de4c31b913ed2 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -1623,62 +1623,68 @@ 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 value if we got one back...
+ 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()) {
+ 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);
+
+ // Cache the value if we got one back...
+ if (!synthetic_child)
+ return ValueObjectSP();
+
+ 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;
}
@@ -1700,13 +1706,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);
@@ -1740,7 +1746,7 @@ ValueObjectSP ValueObject::GetSyntheticBase(uint32_t offset,
return synthetic_child_sp;
if (!can_create)
- return {};
+ return ValueObjectSP();
const bool is_base_class = true;
@@ -1748,7 +1754,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);
@@ -1777,30 +1783,31 @@ 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();
+ auto traversal_none =
+ GetValueForExpressionPathOptions::SyntheticChildrenTraversal::None;
+ path_options.SetSyntheticChildrenTraversal(traversal_none);
+ auto synthetic_child =
+ GetValueForExpressionPath(expression, nullptr, nullptr, path_options);
+
+ // Cache the value if we got one back...
+ if (!synthetic_child)
+ return ValueObjectSP();
+
+ // 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() {
@@ -1992,71 +1999,67 @@ void ValueObject::GetExpressionPath(Stream &s,
}
ValueObjectSP ValueObject::GetValueForExpressionPath(
- llvm::StringRef expression, ExpressionPathScanEndReason *reason_to_stop,
- ExpressionPathEndResultType *final_value_type,
+ llvm::StringRef expression, ExpressionPathScanEndReason *reason_to_stop_ptr,
+ ExpressionPathEndResultType *final_value_type_ptr,
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;
- }
- }
+ ExpressionPathAftermath *final_task_on_target_ptr) {
+
+ auto stop_reason_unknown = eExpressionPathScanEndReasonUnknown;
+ auto value_type_invalid = eExpressionPathEndResultTypeInvalid;
+ auto final_task_nothing = eExpressionPathAftermathNothing;
+
+ auto ret_value = GetValueForExpressionPath_Impl(
+ expression,
+ reason_to_stop_ptr ? reason_to_stop_ptr : &stop_reason_unknown,
+ final_value_type_ptr ? final_value_type_ptr : &value_type_invalid,
+ options,
+ final_task_on_target_ptr ? final_task_on_target_ptr
+ : &final_task_nothing);
+
+ // The caller knows nothing happened if `final_task_on_target` doesn't change.
+ if (!ret_value)
+ return ValueObjectSP();
+
+ if (!final_value_type_ptr)
+ return ret_value;
+
+ if ((*final_value_type_ptr) != eExpressionPathEndResultTypePlain)
+ return ret_value;
+
+ if (!final_task_on_target_ptr)
+ return ret_value;
+
+ ExpressionPathAftermath &final_task_on_target = (*final_task_on_target_ptr);
+ ExpressionPathScanEndReason stop_reason_for_error;
+ Status error;
+ // The method can only dereference and take the address of plain objects.
+ switch (final_task_on_target) {
+ 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 = eExpressionPathAftermathNothing;
+ return ret_value;
+ } else {
+ if (reason_to_stop_ptr)
+ *reason_to_stop_ptr = stop_reason_for_error;
+
+ if (final_value_type_ptr)
+ *final_value_type_ptr = eExpressionPathEndResultTypeInvalid;
+ return ValueObjectSP();
}
- return ret_val; // final_task_on_target will still have its original value, so
- // you know I did not do it
}
ValueObjectSP ValueObject::GetValueForExpressionPath_Impl(
@@ -2727,39 +2730,48 @@ 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