[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

1 files changed, 164 insertions, 166 deletions

diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index 9208047be366..d58bf2ca763d 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) {