Skip to content

[lldb] Improve (for posterity) and then remove unused ValueObject::GetChildAtIndexPath(...) methods #8138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

PortalPete
Copy link

This is a cherry-pick combination of these 2 PRs from llvm/llvm-project:

  1. [lldb] Return index of element in ValueObject path instead of the element's value llvm/llvm-project#74413
  2. [lldb] Remove unused GetChildAtIndexPath(...) methods from ValueObject.cpp llvm/llvm-project#75870

Summary

Part 1 enhances the utility of the method for posterity.
Part 2 removes the method because nothing calls them.

Part 1

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.

Part 2

Nothing calls into these two methods, so we (@adrian-prantl and I) agreed to remove them once we merged the previous PR.

rdar://122447010

…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.
…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.
@PortalPete
Copy link
Author

@swift-ci test

@adrian-prantl adrian-prantl merged commit 608f140 into swiftlang:stable/20230725 Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants