Skip to content

Move BorrowValue to rustpython-common, add BorrowedValue enum#2228

Merged
coolreader18 merged 1 commit into
masterfrom
coolreader18/common-borrowvalue
Sep 23, 2020
Merged

Move BorrowValue to rustpython-common, add BorrowedValue enum#2228
coolreader18 merged 1 commit into
masterfrom
coolreader18/common-borrowvalue

Conversation

@coolreader18

@coolreader18 coolreader18 commented Sep 21, 2020

Copy link
Copy Markdown
Member

I think BorrowedValue would be really useful for #2226, for BufferProtocol::as_bytes() to return

I've had this for a while but the buffer protocol PR made me think this would be useful to merge now

Comment thread common/src/borrow.rs
}

#[derive(Debug, derive_more::From)]
pub enum BorrowedValue<'a, T: ?Sized> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this better to be an enum rather than concrete type or boxed dyn?
I thought buffer protocol help to avoid this situation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I could probably add a Dyn variant that's just Box<dyn Deref> if eventually needed, but as is these are the most common forms of borrowing, and they all just have the underlying borrow stored as a reference/pointer, so no function calls are necessary to Deref it (unlike a dyn Deref)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe I don't know rust pratice well, so this is trying discussion.
When I am seeing BorrowValue for specific type like PyBytesLike, it doesn't use all the enum fields but its borrowed type is BorrowedValue. I think this is giving unnessassary cost.
using enum for each case is ok - because it is what it need to be. But BorrowedValue itself as enum is hiding somewhat not expected to be hided.
my suggestion is, defining specific type for each borrowed types. BorrowedBytesLike or BorrowedRwBytes. And BorrowedValue itself as a trait (if it is needed). We can find better abstraction or we can use dyn BorrowedValue in future complicated scenarios.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this is good because with the buffer protocol PyBytesLike and PyRwBytesLike will become extensible and having a flexible return type from borrow_value will make it easier for implementer's of the buffer protocol. I don't this will actually have much cost as all variants probably will optimize down to a simply deref anyways.

@youknowone

Copy link
Copy Markdown
Member

@qingshi163 will you please review this PR?

@qingshi163 qingshi163 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks nice!

@youknowone

Copy link
Copy Markdown
Member

I just remembered @Skinny121 contributed to PyBytesLike a lot

@coolreader18 coolreader18 merged commit 1263904 into master Sep 23, 2020
@coolreader18 coolreader18 deleted the coolreader18/common-borrowvalue branch September 23, 2020 18:32
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