Rust full reflection#8102
Conversation
dextero
left a comment
There was a problem hiding this comment.
A bunch of small things. Feel free to ignore nits if they don't seem worthwhile.
Great work overall!
|
@candysonya @dextero We are using flatbuffers at work and I have a small patch that allows us to have zero-allocation of flatbuffer messages. I noticed that this change makes all the &'static str into String. Is this strictly necessary? Could it be Cow<'static, str>? |
Thanks for pointing it out! I've just updated the change with |
adsnaider
left a comment
There was a problem hiding this comment.
Thank you for making the changes to Cow<'static, str>. This is an awesome patch set and I'm really excited to have reflection! My comments are a few cases where you used .to_string() but I believe that .into() should work instead to make the value into a Cow and not a String. I could be wrong though and in that case feel free to ignore my comments
Thanks so much for spotting that! |
|
This pull request is stale because it has been open 6 months with no activity. Please comment or label |
|
I do care about this feature. I may pick it up if @candysonya is unavailable. |
|
not-stale |
|
Hi @brt-adam-snaider , yes please feel free to pick it up. Do you already have a plan? |
|
Hi @brt-adam-snaider , I'm considering picking it up. Before that, may I know if you're already working on this? |
|
I haven't picked it up yet. Have at it :) |
dextero
left a comment
There was a problem hiding this comment.
LGTM, thank you for following through on this!
We'll need an owner approval, I can't grant one unfortunately.
|
Some CI issues with Rust. Can you take a look? It should auto run now next time you push to the PR. |
dextero
left a comment
There was a problem hiding this comment.
I left a few nits, mostly about how some errors could be avoided by reorganizing the code a bit. LGTM overall, great work!
|
Looks very nice @candysonya, thank you for your patience! I left a couple more nits and a few general questions about parts of the public API surface of the safe wrapper - we may be able to limit it a little without losing functionality. |
@dextero were the comments sent out? I didn't find them |
|
Whoops, I didn't press The Button. Fixed now! |
It verifies buffer against schema during construction and provides all the unsafe getters in lib.rs in a safe way
dextero
left a comment
There was a problem hiding this comment.
LGTM, great work @candysonya!
(My teammate dextero has kindly helped to review the PR and left a lot of insightful comments. Unfortunately they are not a member of the community and they cannot approve it. Currently all the comments have been resolved and it would be highly appreciated if it could be reviewed by the community because the reflection feature in Rust is a blocker for our team to use Flatbuffers!)
Compared to the C++ implementation of the reflection feature,
It includes:
It doesn't include:
VectorOfAny(Vector without element type specified)TODOs on C++ side are retained.