-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add non scalar default support to BFBS files #8671
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
base: master
Are you sure you want to change the base?
Add non scalar default support to BFBS files #8671
Conversation
|
@dbaileychess and @aardappel interested to hear your collective take on this one! I realized this was missing while working on the wireshark generator. |
|
This PR has formatting and other changes that appear not directly related that make it hard to review. Why is the new bool in |
|
This is getting at the inelegance of the design -- much of the code relies on the constant string being default constructed as I tried to just default construct constant and set it to 0 if a scalar type, but then I couldn't tell the difference between "user set default to |
|
in reading up on the non scalar defaults more (#6053), it does seem that the bfbs would need to differentiate between being provided a default (
1 or 3 would work for me, and I can work to remove the auto formatting to make the pr easier to review (vscode format go brrrrrr) |
148d925 to
d92b677
Compare
|
there were only two lines of formatting changes -- the other change is flatc now generates scoped enums instead of unscoped. |
|
@dbaileychess your review here would be helpful |
ca5d722 to
22fa6e2
Compare
22fa6e2 to
64f8e55
Compare
64f8e55 to
bf03023
Compare
|
not 100% ready for merge but want to get it into the review pipeline |
| padding:uint16 = 0; | ||
| /// If the field uses 64-bit offsets. | ||
| offset64:bool = false; | ||
| default_non_scalar:string; |
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.
I feel like a better long term solution is to have a default_value field that is of type Union (long, string, short, etc..)
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.
Would you want me to deprecate the old constant field to do so?
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.
This would also have to be a union of struct of primitive, since unions of primitives aren't a thing
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.
Probably not great to remove the existing fields.. so we may as well continue with the current pattern?
Just not great to merge strings and vectors into 1 field. Better probably to have default:string (which at least in FlatBuffers can be detected as not-set vs empty) and default_vector:bool` ?
In terms of the variable constant in the parser.. I actually like your idea of making it contain the string including extra "": this would make a lot of sense as way to represent different types of values inside a string.
The long term solution would be to replace constant with a variant<int64_t,double,string,bool> or similar? But that will likely cause a LOT of code churn.
|
I will see about refactoring this to use option 3 |
This PR adds in an extra field to the reflection.fbs that holds string representations of non scalar defaults -- vectors and strings, specifically.
This isn't the 'best' way to accomplish this, but I think its the most pragmatic given how prolific the
Valuestruct is in the code base.Accepting suggestions for better names!