[lldb] Return index of element in ValueObject path instead of the element's value #74413

PortalPete · 2023-12-05T05:44:55Z

It's more meaningful and actionable to indicate which element in the array has an issue by returning that element's index instead of its value. The value can be ambiguous if at least one other element has the same value.

The first parameter for these methods is idxs, an array of indices that represent a path from a (root) parent to on of its descendants, typically though intermediate descendants. When the path leads to a descendant that doesn't exist, the method is supposed to indicate where things went wrong by setting an index to &index_of_error, the second parameter.

The problem is the method sets *index_of_error to the index of the most recent parent's child in the hierarchy, which isn't very useful if there's more one index with the same value in the path.

In this example, each element in the path has a value that's the same as another element.

GetChildAtIndexPath({1, 2, 3, 3, 1, 1, 2}, &index_of_error);

Say the the second 1 in the path (the 5th element at [4]) doesn't exist and the code returns a nullptr. In that situation, the code sets *index_of_error to 1, but that's an ambiguous hint can implicate the 1st, 5th, or 6th element (at [0], [4], or [5]).

It’s more helpful to set *index_of_error to 4 to clearly indicate which element in idxs has the issue.

llvmbot · 2023-12-05T05:52:14Z

@llvm/pr-subscribers-lldb

Author: Pete Lawrence (PortalPete)

Changes

rdar://119169160

Full diff: https://github.com/llvm/llvm-project/pull/74413.diff

1 Files Affected:

(modified) lldb/source/Core/ValueObject.cpp (+9-2)

diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index a7f7ee64282d8..b13bffa0ca809 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -398,13 +398,16 @@ ValueObject::GetChildAtIndexPath(llvm::ArrayRef<size_t> idxs,
   if (idxs.size() == 0)
     return GetSP();
   ValueObjectSP root(GetSP());
+
+  size_t current_index = 0;
   for (size_t idx : idxs) {
     root = root->GetChildAtIndex(idx);
     if (!root) {
       if (index_of_error)
-        *index_of_error = idx;
+        *index_of_error = current_index;
       return root;
     }
+    current_index += 1;
   }
   return root;
 }
@@ -414,13 +417,17 @@ lldb::ValueObjectSP ValueObject::GetChildAtIndexPath(
   if (idxs.size() == 0)
     return GetSP();
   ValueObjectSP root(GetSP());
+
+  size_t current_index = 0;
   for (std::pair<size_t, bool> idx : idxs) {
     root = root->GetChildAtIndex(idx.first, idx.second);
     if (!root) {
       if (index_of_error)
-        *index_of_error = idx.first;
+        *index_of_error = current_index;
       return root;
     }
+
+    current_index += 1;
   }
   return root;
 }

DavidSpickett · 2023-12-05T11:33:28Z

I can see why the mistake was made, we have an index into a list of different indexes? Anyway, please add a test for this.

chelcassanova · 2023-12-05T18:09:46Z

lldb/source/Core/ValueObject.cpp

      return root;
    }
+    current_index += 1;


This is the tiniest nit on earth but current_index++ here to increment this?

Sure!
I had a hunch someone would point this one out, but I thought it'd be fun to see who, if anyone, would.
You passed the test! 😂

Personally, I prefer += 1 because:

Swift doesn't have the ++ operator.

In my mind += 1 more explicitly communicates my intent.

I've seen people get burned by the "post" part of a post-increment operator too many times. 🙃

After reading the Swift rationale for removing the ++ operator I understand why you'd use += 1 instead.

I take it you're referring to Swift evolution's proposal 0004 - Remove the ++ and -- operators.

As a coding and computer science mentor and writer, I naturally agree with Chris Lattner in that any code I write may serves as potential educational material to the reader/maintainer. For me, there are more cons than pros to the pre- and post-increment operators when I look at them through an educational lens.

felipepiovezan · 2023-12-05T18:10:41Z

Should we change the documentation of these functions?

chelcassanova · 2023-12-05T18:55:40Z

lldb/source/Core/ValueObject.cpp

      return root;
    }
+
+    current_index += 1;


Ditto for current_index++

I believe I'll use the pre-increment operator ++current_index because that's what the LLVM coding style recommends.

Hard fast rule: Preincrement (++X) may be no slower than postincrement (X++) and could very well be a lot faster than it. Use preincrementation whenever possible.

...

Note this guideline is about pre vs post (not about +=1), and it refers to iterators mostly.

I don't think you need to change anything here!

Right, but if we're not going with += 1 (for consistency and/or informal style?), then I prefer to use the pre-increment operator.

Or are you saying we should just stick with += 1 now?

Ok, I'll just stick with += 1 and hope it becomes a Swift-inspired trend.

PortalPete · 2023-12-08T00:34:58Z

I can see why the mistake was made, we have an index into a list of different indexes? Anyway, please add a test for this.

I'm not really sure there's a good way to test this there doesn't appear to be anything that calls these methods, let alone an SB API.

CC @adrian-prantl

…ment's value It's more meaningful and actionable to indicate which element in the array has an issue by returning that element's index instead of its value. The value can be ambiguous if at least one other element has the same value. The first parameter for these methods is `idxs`, an array of indices that represent a path from a (root) parent to on of its descendants, typically though intermediate descendants. When the path leads to a descendant that doesn't exist, the method is supposed to indicate where things went wrong by setting an index to `&index_of_error`, the second parameter. The problem is the method sets `*index_of_error` to the index of the most recent parent's child in the hierarchy, which isn't very useful if there's more one index with the same value in the path. In this example, each element in the path has a value that's the same as another element. ```cpp GetChildAtIndexPath({1, 2, 3, 3, 1, 1, 2}, &index_of_error); ``` Say the the second `1` in the path (the 5th element at `[4]`) doesn't exist and the code returns a `nullptr`. In that situation, the code sets `*index_of_error` to `1`, but that's an ambiguous hint can implicate the 1st, 5th, or 6th element (at `[0]`, `[4]`, or `[5]`). It’s more helpful to set `*index_of_error` to `4` to clearly indicate which element in `idxs` has the issue. rdar://119169160

DavidSpickett · 2023-12-08T09:54:45Z

doesn't appear to be anything that calls these methods

Candidate for removal then? If so it still makes sense to fix them, then remove them. Saves fixing them again if they are brought back.

Either way, I'm not going to block this by requiring a test case.

adrian-prantl · 2023-12-08T16:46:49Z

doesn't appear to be anything that calls these methods

Candidate for removal then? If so it still makes sense to fix them, then remove them. Saves fixing them again if they are brought back.

Either way, I'm not going to block this by requiring a test case.

I think that makes sense. @PortalPete why don't you land this without a test and then create a PR to remove the unused methods?

PortalPete · 2023-12-08T18:51:15Z

doesn't appear to be anything that calls these methods

Candidate for removal then? If so it still makes sense to fix them, then remove them. Saves fixing them again if they are brought back.
Either way, I'm not going to block this by requiring a test case.

I think that makes sense. @PortalPete why don't you land this without a test and then create a PR to remove the unused methods?

That works for me, @adrian-prantl.
I'll remove these methods wholesale after someone approves/merges this PR on my behalf.

@DavidSpickett

…t.cpp (#75870) This a follow-up PR from this other one: #74413 Nothing calls into these two methods, so we (@DavidSpickett, @adrian-prantl, and I) agreed to remove them once we merged the previous PR.

…ment's value (llvm#74413) It's more meaningful and actionable to indicate which element in the array has an issue by returning that element's index instead of its value. The value can be ambiguous if at least one other element has the same value. The first parameter for these methods is `idxs`, an array of indices that represent a path from a (root) parent to on of its descendants, typically though intermediate descendants. When the path leads to a descendant that doesn't exist, the method is supposed to indicate where things went wrong by setting an index to `&index_of_error`, the second parameter. The problem is the method sets `*index_of_error` to the index of the most recent parent's child in the hierarchy, which isn't very useful if there's more one index with the same value in the path. In this example, each element in the path has a value that's the same as another element. ```cpp GetChildAtIndexPath({1, 2, 3, 3, 1, 1, 2}, &index_of_error); ``` Say the the second `1` in the path (the 5th element at `[4]`) doesn't exist and the code returns a `nullptr`. In that situation, the code sets `*index_of_error` to `1`, but that's an ambiguous hint can implicate the 1st, 5th, or 6th element (at `[0]`, `[4]`, or `[5]`). It’s more helpful to set `*index_of_error` to `4` to clearly indicate which element in `idxs` has the issue.

@DavidSpickett

…t.cpp (llvm#75870) This a follow-up PR from this other one: llvm#74413 Nothing calls into these two methods, so we (@DavidSpickett, @adrian-prantl, and I) agreed to remove them once we merged the previous PR.

@DavidSpickett

…t.cpp (#75870) This a follow-up PR from this other one: llvm/llvm-project#74413 Nothing calls into these two methods, so we (@DavidSpickett, @adrian-prantl, and I) agreed to remove them once we merged the previous PR.

PortalPete marked this pull request as ready for review December 5, 2023 05:51

PortalPete requested a review from JDevlieghere as a code owner December 5, 2023 05:51

llvmbot added the lldb label Dec 5, 2023

chelcassanova reviewed Dec 5, 2023

View reviewed changes

PortalPete force-pushed the misc/valueobject-index-path branch from 26891e6 to 5d99172 Compare December 6, 2023 00:10

PortalPete force-pushed the misc/valueobject-index-path branch from 5d99172 to 5731517 Compare December 8, 2023 00:43

felipepiovezan approved these changes Dec 8, 2023

View reviewed changes

adrian-prantl merged commit c155269 into llvm:main Dec 13, 2023

PortalPete mentioned this pull request Dec 18, 2023

[lldb] Remove unused GetChildAtIndexPath(...) methods from ValueObject.cpp #75870

Merged

PortalPete mentioned this pull request Feb 7, 2024

[lldb] Improve (for posterity) and then remove unused ValueObject::GetChildAtIndexPath(...) methods swiftlang/llvm-project#8138

Merged

[lldb] Return index of element in ValueObject path instead of the element's value #74413

[lldb] Return index of element in ValueObject path instead of the element's value #74413

Uh oh!

Conversation

PortalPete commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 5, 2023

Uh oh!

DavidSpickett commented Dec 5, 2023

Uh oh!

chelcassanova Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

PortalPete Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

chelcassanova Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

PortalPete Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

felipepiovezan commented Dec 5, 2023

Uh oh!

chelcassanova Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

PortalPete Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PortalPete Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

PortalPete Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

PortalPete commented Dec 8, 2023

Uh oh!

DavidSpickett commented Dec 8, 2023

Uh oh!

adrian-prantl commented Dec 8, 2023

Uh oh!

PortalPete commented Dec 8, 2023

Uh oh!

Uh oh!

PortalPete commented Dec 5, 2023 •

edited

Loading

PortalPete Dec 5, 2023 •

edited

Loading

felipepiovezan Dec 5, 2023 •

edited

Loading