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 as_tuple() method to PyList #3042

Merged
merged 3 commits into from
Mar 12, 2023
Merged

add as_tuple() method to PyList #3042

merged 3 commits into from
Mar 12, 2023

Conversation

samuelcolvin
Copy link
Contributor

From benchmarks, this is significantly faster than PyTuple::new(py, the_list):

test list_as_tuple_direct        ... bench:         299 ns/iter (+/- 51)
test list_as_tuple_iterate       ... bench:         521 ns/iter (+/- 41)

.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks good other than the change on the .gitignore which I'm not convinced is necessary!

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 12, 2023

Build succeeded:

@bors bors bot merged commit 30521c8 into PyO3:main Mar 12, 2023
@mejrs
Copy link
Member

mejrs commented Mar 12, 2023

I would prefer a From<&PyList> for &PyTuple implementation.

@samuelcolvin samuelcolvin deleted the list-as-tuple branch March 12, 2023 14:49
@samuelcolvin
Copy link
Contributor Author

I would prefer a From<&PyList> for &PyTuple implementation.

I see that but it would be less obvious what's going on, and why the reverse can't be done.

@davidhewitt
Copy link
Member

davidhewitt commented Mar 12, 2023

I would prefer a From<&PyList> for &PyTuple implementation.

Actually this is a great point, I think the reverse could be implemented by tuple.as_sequence().list(). However I would like to ask we put this on the backlog for the moment just because with reboot of #1308 there would be ownership difference and then it's less clear if the implementation should be From<&'_ PyList<'py>> for PyTuple<'py> or From<PyList<'py>> for PyTuple<'py> (the difference being whether a reference to the PyList is borrowed or consumed).

However this also makes me notice another snag, which is naming convention. This is as_tuple() (and matches the C API name) but in Rust the as_ methods are usually zero-cost conversions, with to_ conversions being more costly e.g. to_string vs as_str. So I think there's a few considerations:

  • Call this just .tuple() to match PySequence::tuple().
  • Call this to_tuple() and rename sequence methods to PySequence::to_tuple() and PySequence::to_list().
    • I'm tempted to open a PR to rename the sequence methods anyway, I think the to_ prefix may be strictly better.
  • Revert this and recommend just using the sequence API. It looks like PySequence_Tuple has a special case to call PyList_AsTuple so there would be negligible perf difference.

@mejrs
Copy link
Member

mejrs commented Mar 12, 2023

The to_* renaming seems like a good idea

@samuelcolvin
Copy link
Contributor Author

👍 for to_*.

@davidhewitt
Copy link
Member

Pushed #3043 and I'll add separate PRs for other renaming and also PyTuple::to_list when I get a moment.

bors bot added a commit that referenced this pull request Mar 12, 2023
3043: `PyList::as_tuple()` -> `PyList::to_tuple()` r=mejrs a=davidhewitt

As discussed as a follow-up to #3042 

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
bors bot added a commit that referenced this pull request Mar 12, 2023
3043: `PyList::as_tuple()` -> `PyList::to_tuple()` r=mejrs a=davidhewitt

As discussed as a follow-up to #3042 

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
bors bot added a commit that referenced this pull request Apr 18, 2023
3111: Rename sequence `.list()` and `.tuple()` to `.to_list()` and `.to_tuple()` r=adamreichold a=davidhewitt

As agreed in #3042 (comment).

The motivation is that it is an emerging Rust convention for `to_x` to mean "a non-consuming conversion to type x which has some (small) overhead". E.g. `to_string`, `to_owned`.

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
bors bot added a commit that referenced this pull request Apr 19, 2023
3111: Rename sequence `.list()` and `.tuple()` to `.to_list()` and `.to_tuple()` r=adamreichold a=davidhewitt

As agreed in #3042 (comment).

The motivation is that it is an emerging Rust convention for `to_x` to mean "a non-consuming conversion to type x which has some (small) overhead". E.g. `to_string`, `to_owned`.

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
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.

4 participants