Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 13, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lldb/include/lldb/Core/ValueObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,15 +468,15 @@ class ValueObject {
virtual lldb::ValueObjectSP GetChildAtIndex(size_t idx,
bool can_create = true);

// this will always create the children if necessary
// The method always creates missing children in the path, if necessary.
lldb::ValueObjectSP GetChildAtIndexPath(llvm::ArrayRef<size_t> idxs,
size_t *index_of_error = nullptr);

lldb::ValueObjectSP
GetChildAtIndexPath(llvm::ArrayRef<std::pair<size_t, bool>> idxs,
size_t *index_of_error = nullptr);

// this will always create the children if necessary
// The method always creates missing children in the path, if necessary.
lldb::ValueObjectSP GetChildAtNamePath(llvm::ArrayRef<llvm::StringRef> names);

virtual lldb::ValueObjectSP GetChildMemberWithName(llvm::StringRef name,
Expand Down
11 changes: 9 additions & 2 deletions lldb/source/Core/ValueObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙃

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return root;
}
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto for current_index++

Copy link
Contributor Author

@PortalPete PortalPete Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

...

Copy link
Contributor

@felipepiovezan felipepiovezan Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return root;
}
Expand Down