-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Support native_type for tables when using the C++ object API. #8668
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
Conversation
|
This generally looks good, wonder if this does what people on the original issue wanted it for @buster3 @iceboy233 @sketch34 @lp35 @meniku |
de16b9f to
f6c8b40
Compare
969d890 to
c112473
Compare
c112473 to
81b9789
Compare
|
Can you turn on |
db1df79 to
dbf2cce
Compare
Added. This was not working at main prior to this change. I'm also not sure the behavior for structs was correct: when a flatbuffer struct was annotated with native_type, it was generating a comparator for the flatbuffer struct. I think the desired behavior would be to only generate comparators for the object API generated types (suffixed with T), and not generate comparators for structs/tables with native_types (which need to define the comparators themselves). Let me know if this interpretation is correct (per the flatc description: |
Maybe it can be a separate change as it's not working prior. Is it feasible/align with existing code to allow user to implement the member |
Sounds good. Mailed #8681 for this.
Happy to do whatever you and @aardappel suggest. Let me try to draft. I think this will just reverse some of the current dependencies of autogenerated Pack() -> Create(), and require rewriting some of the autogenerated object API type code to use Pack instead of Create. |
|
@dbaileychess opinion? |
9aa8be6 to
34f48ba
Compare
@aardappel @dbaileychess I mailed #8754 which reverses the dependency of CreateX and X::Pack methods, which will allow users to define X::Pack (instead of CreateX, which will then call X::Pack) in order to provide a table native_type implementation. |
aardappel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM!
Thanks! I was hoping we could check in #8681 first (to fix the upstream gen compare issue). And then I will rebase this one and switch it to use the Pack method. |
If native_type is specified on a table: - No object API struct type is generated. - The object API refers to the table by its native_type. - UnPack and Create<TableName> methods are declared but not defined; as they must be user-provided.
2c9ef46 to
55b011f
Compare
55b011f to
0fdeeb6
Compare
0fdeeb6 to
7454517
Compare
|
@aardappel rebased onto master w/ the previous two changes, and ready for review. Thank you! |
|
thanks! |
If native_type is specified on a table:
must be user-provided.
Addresses #4969