llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Pete Lawrence (PortalPete) <details> <summary>Changes</summary> … 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 --- Full diff: https://github.com/llvm/llvm-project/pull/75865.diff 1 Files Affected: - (modified) lldb/source/Core/ValueObject.cpp (+178-166) ``````````diff 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) { `````````` </details> https://github.com/llvm/llvm-project/pull/75865 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
