[Lldb-commits] [lldb] [lldb] Improve maintainability and readability for ValueObject methods (PR #75865)

[Alex Langford via lldb-commits]

================
@@ -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 =
----------------
bulbazord wrote:

(This is my opinion, let me know if you disagree)
I initially suggested this because putting something in its own variable helps with readability when:
- You're going to use this value over and over, putting it in its own variable helps with readability and refactorability.
- You're trying to take something confusing and make something about it clearer. I usually do this in two scenarios:
  1. Computing complex expressions. Pulling it out into its own variable means you get to give the complex expression a "meaning". It allows you to build a mental model of what the code is trying to do, not how it's doing it.
  2. Making the type of something explicit. For example, if the type of something feels ambiguous through reading it (though the compiler does resolve a type for the expression), putting it into its own variable can help improve readability. This one is less of a concern with good code editing tooling (like most IDEs provide), but still to me since I read a lot of code without an IDE.

I don't hold this opinion very strongly though. What do you think?

https://github.com/llvm/llvm-project/pull/75865