Include services in reflection data (fixes #4639)#4713
Include services in reflection data (fixes #4639)#4713aardappel merged 3 commits intogoogle:masterfrom xbr:expand_reflection
Conversation
oberstet
changed the title
aardappel
left a comment
aardappel
left a comment
There was a problem hiding this comment.
Mostly looks good! A few issues..
include/flatbuffers/idl.h
Outdated
| struct RPCCall : public Definition { | ||
| Offset<reflection::RPCCall> Serialize(FlatBufferBuilder *builder, const Parser &parser) const; | ||
|
|
||
| std::string name; |
There was a problem hiding this comment.
Definition also contains a name and doc_comment already, so these can be removed below.
There was a problem hiding this comment.
=> attributes removed (note: the use of doc_comment from the base class instead of the dedicated and now removed rpc_comment triggered small changes elsewhere)
| @@ -44,6 +44,7 @@ table EnumVal { | |||
| value:long (key); | |||
There was a problem hiding this comment.
why is generate_code.sh changed (can't comment on that actual file)
There was a problem hiding this comment.
I added executable flag (as with most other scripts in the repo)
|
|
||
| table RPCCall { | ||
| name:string (required, key); | ||
| request:Object (required); // must be a table (not a struct) |
There was a problem hiding this comment.
Having these two being an Object requires RPCCalls to be serialized after objects.. which is fine I guess. An alternative would be an index into objects much like elsewhere.
There was a problem hiding this comment.
I don't understand what you mean here. I tried to follow closely the rest of the model - and the model works (I am using the new stuff, a bfbs file generated, reading from Python). Could you expand? What is the concern, or how would I write the reflection schema differently?
There was a problem hiding this comment.
There is no problem, merely noting the consequences of this choice.
There was a problem hiding this comment.
I like the idea of it being an index into objects(). Otherwise this would duplicate some data serialized into objects, correct?
There was a problem hiding this comment.
@englercj No, this makes no difference in terms of size, since objects are serialized once regardless.
reflection/reflection.fbs
Outdated
| enums:[Enum] (required); // Sorted. | ||
| objects:[Object] (required); // Sorted. | ||
| enums:[Enum] (required); // Sorted. | ||
| services:[Service]; // Sorted. |
There was a problem hiding this comment.
Inserting fields in the middle like this is an incompatible change and would make this schema incompatible with all pre-existing binary schema files. Add at the end, or give all fields a correct id attribute.
There was a problem hiding this comment.
uups. moved services to the end
src/flatc.cpp
Outdated
| " --grpc Generate GRPC interfaces for the specified languages\n" | ||
| " --schema Serialize schemas instead of JSON (use with -b)\n" | ||
| " --bfbs-comments Add doc comments to the binary schema files.\n" | ||
| " --bfbs-builtin-attrs Add builtin attributes to the binary schema files.\n" |
There was a problem hiding this comment.
I hate to ask, but can we make this flag shorter so it lines up? :(
There was a problem hiding this comment.
renamed to --bfbs-builtins (there is a tradeoff here;)
| IsInteger(value.type.base_type) ? StringToInt(value.constant.c_str()) : 0, | ||
| IsFloat(value.type.base_type) ? strtod(value.constant.c_str(), nullptr) | ||
| : 0.0, | ||
| deprecated, required, key, SerializeAttributes(builder, parser), |
There was a problem hiding this comment.
was this caused by clang-format? if not, prefer not to reformat unchanged code.
There was a problem hiding this comment.
no, this was caused by me not reverting my random reformats as I hacked on the code;) => reverted
|
CI shows a segfault in the tests, can you investigate? https://travis-ci.org/google/flatbuffers/jobs/370278046 |
|
CI passes now: forgot to regenerate the test data |
|
@aardappel ping: anything else I should do rgd the PR? thx! |
|
Looks good, thanks! |
* include service in reflection data (fixes google#4639) * changes from review * regenerated test data
3 participants
supersedes #4711 : squashed
the main change: expanding the reflection schema https://github.com/google/flatbuffers/pull/4713/files#diff-db35d829e5e236af29f9a061c8352dcb