search

tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
452 stars 87 forks source link

Rentals #139

Closed nerdrew closed 5 years ago

tafia commented 5 years ago

I am not super familiar with rental but it looks to be working fine. Do you have any particular comment?

nerdrew commented 5 years ago

Sorry for the delay. I don't have any particular comments. I've been using this code for a while in an app that I've been playing with, but it hasn't ever been deployed anywhere.

edit: jk, I did have a comment :p

nerdrew commented 5 years ago

I have just one request though, would you mind defaulting to using StructNameBorrow/StructName (or something with similar meaning for borrow) instead of StructName/SructNameRental because I believe that users using this rental flag will likely use the Rental version more than the borrowed one.

Just to clarify: use the StructNameBorrow/StructName naming only when the rental flag is enabled? When the rental flag is disabled, it would still be StructName? Or do you want me to rename the existing StructName to StructNameBorrow in all cases?

tafia commented 5 years ago

Sorry I wasn't clear. The renaming is only when using the flag, there should be no change if the flag isn't used.

nerdrew commented 5 years ago

Further clarification needed: are you thinking just the structs that have a rental generated (structs that have lifetimes) would get the Borrow suffix? Would different names based on if the struct has a lifetime be confusing?

One concern (I implemented the rename, and now I'm dealing with tests...): enabling the rental flag will change a lot of struct names, which could require significant refactoring for apps.

Thoughts? You can see the test change here: https://github.com/tafia/quick-protobuf/pull/139/commits/f934185c33b871390a695c6ccfcbce7cba3c219b#diff-fe280ad72fcec9a84768d8074ea027ad

edit: The commit I pushed has all structs renamed to <Struct>Borrow, not just the structs with lifetimes.

tafia commented 5 years ago

Yes if possible only those with lifetime. I don't think it'll be too much confusing. In practice I bet they won't use the Borrow version much so rental is an implementation detail.

This will be a breaking change so people opting in for rental should be ready to face the necessary changes.

Thanks again

tafia commented 5 years ago

I'm curious to know what is your opinion by the way

nerdrew commented 5 years ago

I see arguments both ways. When reading protos in my app, I always use the Owned / Rental struct. When writing protos, I always use the Borrowed version. I guess it depends on what an app is doing with protos to see what people will use more. My app appears to use them roughly evenly, but it's a webapp doing request response cycles, so I guess that's expected. (My tests use the Owned version way more.)

I guess the analogy is String => &str or PathBuf => Path:

FooMessage => FooMessageBorrowed vs FooMessageOwned => FooMessage. Not sure which is better.

Side note: the FooMessageRental suffix isn't very descriptive, FooMessageOwned or something similar would be better if we go back to that route.

nerdrew commented 5 years ago

Another question: should the flag be --owned or something similar too to make it more descriptive?

tafia commented 5 years ago

I like the --owned/Owned variant. I agree with the analogy str/String there is not really a better choice, it depends on your scenario. Let's keep it simple then just use owned for rentals.

Sorry about all this.

nerdrew commented 5 years ago

No problem. I'll update tonight. Better to talk through the API before merging :)

nerdrew commented 5 years ago

Ok, another day late, but pushed my update. Take a look. What do you think?

nerdrew commented 5 years ago

I have a number of commits in here. I'll squash them before you merge to make the history cleaner. Leaving the commits separate to (hopefully) make reviewing easier.

nerdrew commented 5 years ago

Hmmm. Based on this https://github.com/jpernst/rental/issues/34 we might want to wait on merging this. If the rental crate is in maintenance mode, you might not want to add it as a dependency. But... it looks like the standard library recently added support for doing the same thing in a supported manner: https://doc.rust-lang.org/std/pin/index.html#example-self-referential-struct. Are you ok with bumping the required rustc version for this crate? If so, I'll take a look at re-implementing this PR using Pin instead of rental.

tafia commented 5 years ago

I'm totally ok to bump the required version for this crate! Thanks a lot!

nerdrew commented 5 years ago

Ok. I have another version of owned structs that doesn't use rentals. It has a couple unsafe blocks to make self-referential structs work. Should I push to this branch or do you want me to open a new PR so we can compare the 2 PRs?

tafia commented 5 years ago

If you don't mind I'd prefer having 2 branches. Thanks

nerdrew commented 5 years ago

https://github.com/tafia/quick-protobuf/pull/143

nerdrew commented 5 years ago

The owned branch was merged instead.