Dart object API#6682
Conversation
|
|
||
| int _pack(fb.Builder fbBuilder, Map<Object, int> existingOffsets) { | ||
| assert(existingOffsets != null); | ||
| assert(fbBuilder != null); |
There was a problem hiding this comment.
this seems a bit superfluous in generated code.. most callers are other bits of generated code, and the user calling it with a null builder doesn't seem very likely.
There was a problem hiding this comment.
FWIW, these are stripped away in non-debug modes (and can be stripped away even in debug mode).
However, if it's likely that this indicates an error in the generated code rather than in the caller, it'd be good to at least provide a message to that effect. This also can go away whenever we get to migrate this code to NNBD, which I think someone is working on.
There was a problem hiding this comment.
Again as mentioned above - keeping in line with the previous ObjectBuilder code. Also, it doesn't hurt anything and is really only a temporary piece of code to be removed with null-safety.
That "someone" working on null-safety (or rather porting it from my work in the ObjectBox fork of flatbuffers) is me, at least I've said I would but didn't get to it yet. But it is on my list soon-ish, hopefully this PR will be merged by the time I do that, to avoid code conflicts.
|
|
||
| fbBuilder.pad(2); | ||
| test3.pack(fbBuilder); | ||
| fbBuilder.pad(1); |
There was a problem hiding this comment.
the fact that its manually padding to me means that it is serializing manually rather than use the methods from the base API to serialize which presumably already pad for you? better to use those?
There was a problem hiding this comment.
Yet again, code from the existing ObjectBuilder.finish() - should - do you want to touch that, i.e. should I have a look why it doesn't use some existing method there?
There was a problem hiding this comment.
well, we shouldn't copy existing code from the base API, we should be calling into it.
There was a problem hiding this comment.
It is existing generated code, not FlatBuffers API, and unfortunately can't be reused directly (except on the generator side, where I do it) because of a couple of differences, like how inner tables/structs are provided. I can do that but I'd probably have to change some of the generated code unrelated to ObjectAPIs - is that how you'd prefer to do this instead?
| final int idOffset = fbBuilder.writeString(id); | ||
|
|
||
| fbBuilder.startTable(); | ||
| if (idOffset != null) { |
There was a problem hiding this comment.
compares integer with null ?
There was a problem hiding this comment.
ints are nullable in dart, unless you're in null-safety mode in which case it'd be written as int?.
I don't think we should ever expect writeString to return null - for example if the buffer is out of space wouldn't we just throw?
There was a problem hiding this comment.
writeString() returns null if the given string (value) is null itself.
|
|
||
| int _pack(fb.Builder fbBuilder, Map<Object, int> existingOffsets) { | ||
| assert(existingOffsets != null); | ||
| assert(fbBuilder != null); |
There was a problem hiding this comment.
FWIW, these are stripped away in non-debug modes (and can be stripped away even in debug mode).
However, if it's likely that this indicates an error in the generated code rather than in the caller, it'd be good to at least provide a message to that effect. This also can go away whenever we get to migrate this code to NNBD, which I think someone is working on.
| fbBuilder.startTable(); | ||
| return fbBuilder.endTable(); |
There was a problem hiding this comment.
Why does this just create an empty table?
There was a problem hiding this comment.
There are no properties. Yet again, see existing ObjectBuilder.finish()
| final int idOffset = fbBuilder.writeString(id); | ||
|
|
||
| fbBuilder.startTable(); | ||
| if (idOffset != null) { |
There was a problem hiding this comment.
ints are nullable in dart, unless you're in null-safety mode in which case it'd be written as int?.
I don't think we should ever expect writeString to return null - for example if the buffer is out of space wouldn't we just throw?
|
Needs tests :) |
|
Added tests but they're blocked by an incorrect struct building I've found in the original code (i.e. unrelated to the changes in this branch). For that reason, part of the test case comparing the actually written and read data is commented out. After #6688 is resolved, this test case can be re-enabled and should pass. |
|
Anything I should change for this to get merged? @dnfield @aardappel Some discussions above can be resolved already? |
…heck-generated-code.sh
|
I've fixed the compilation on old MSVC with c++0x as reported by AppVeyor CI. Is there anything else to do here before the merge @aardappel? |
|
Nice, thanks! |
3 participants
closes #6678