Skip to content
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

Add search methods for packed arrays #60855

Merged
merged 1 commit into from
May 10, 2022

Conversation

timothyqiu
Copy link
Member

Adds the following methods to packed arrays:

  • count()
  • find()
  • rfind()

They are useful because:

  • remove_at() requires an index. Currently, only bsearch() provides an index and it requires to sort the array first.
  • Array already provides these methods.

* count()
* find()
* rfind()
@kleonc
Copy link
Member

kleonc commented May 7, 2022

I like adding these methods but I think both find() and rfind() should treat their parameters in the same way. For me treating either [0, n - 1] or [-n, n - 1] as valid index range would be acceptable.

<argument index="0" name="value" type="int" />
<argument index="1" name="from" type="int" default="-1" />
<description>
Searches the array in reverse order. Optionally, a start search index can be passed. If negative, the start index is considered relative to the end of the array.
Copy link
Member

Choose a reason for hiding this comment

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

Also this is not true for the current implementation. For an array with 2 elements -5 (if considered as relative to the end of array) is out of bounds. But with the current implementation it would just search the whole array (and the same would happen for positive values out of bounds).

@timothyqiu
Copy link
Member Author

Yeah, I thought about those too. But this is currently how the corresponding methods in plain Array works.

Do you think I should change the Array implementation & doc in this PR too?

@kleonc
Copy link
Member

kleonc commented May 7, 2022

Do you think I should change the Array implementation & doc in this PR too?

I do think these methods should be changed to behave the same across these classes. But I'd say firstly there should be consensus about what the proper/expected behavior is. If there would be such consensus then sure, I don't have anything against adding such changes in this PR.

On the other hand I guess if it was made like that in the Array then it was somehow accepted? 🤔 So maybe behavior in this PR can remain as is as it's consistent with already existing inconsistency (in the Array). And that whole find()/rfind() inconsistency could (and should) be fixed in another PR.

@timothyqiu
Copy link
Member Author

I created a issue for the current index handling inconsistency among array methods: #60878

Let's fix that in another PR.

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

Added docs for rfind() are still incorrect here but I guess they can be also fixed in another PR (the one making find()/rfind() consistent).

@akien-mga akien-mga merged commit cc66d5e into godotengine:master May 10, 2022
@akien-mga
Copy link
Member

Thanks!

@timothyqiu timothyqiu deleted the packed-array-find branch May 10, 2022 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants