[Lldb-commits] [lldb] [lldb] Improve maintainability and readability (and behavior?)… (PR #75865)

via lldb-commits lldb-commits at lists.llvm.org
Mon Dec 18 14:40:25 PST 2023


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


More information about the lldb-commits mailing list