Add test for nested buffer verifier #7252
Conversation
tests/test.cpp
Outdated
| MonsterT monster; | ||
|
|
||
| // Set the nested monster bytes. | ||
| monster.testnestedflatbuffer = std::move(nested_monster_bytes); |
There was a problem hiding this comment.
I guess this is nice to test the Object API, but this really makes 2 (!) unnecessary copies (first the nested_monster_bytes above, then the copy from the MonsterT into the real flatbuffer.. if you hadn't used std::move here it would be 3 copies).
That shouldn't matter for a test, but really it is good to use "idiomatic" (which for flatbuffers means "efficient") code where possible. Not using the object API, you could serialize the bytes pointer to by GetBufferPointer straight into the parent buffer without extra copies.
There was a problem hiding this comment.
Yeah, I was focused on what I was testing more than the efficiency. I was aiming for conceptual efficiency more than runtime.
If you want me to change it to using the standard API, I can.
There was a problem hiding this comment.
This got me thinking too, I didn't realize that testnestedflatbuffer only took a std::vector<uint8_t>, it would be nice to take in a MonsterT * directly and avoid the copy.
There was a problem hiding this comment.
I don't think it be much more using the standard API.
A nested FlatBuffer taking a MonsterT * wouldn't avoid a copy, since it has to first serialize that to bytes before it can add that. We'd could avoid the copy if we serialized the inner one with a special allocator that knows how to use the outer one as a buffer, assuming alignment works out etc.. but that doesn't make sense to do this for the Object API since that is already a copy and is already less efficient. If we wanted to have this kind of in-place nested construction, we should make that for the base API, and then the Object API could follow.
There was a problem hiding this comment.
Switched to the base API. Its only annoying since the name field is required, so you have to do a bit of extra work to make valid.
| @@ -227,7 +227,7 @@ flatbuffers::DetachedBuffer CreateFlatBufferTest(std::string &buffer) { | |||
|
|
|||
| FinishMonsterBuffer(builder, mloc); | |||
|
|
|||
| // clang-format off | |||
| // clang-format off | |||
There was a problem hiding this comment.
I formatted the whole file, and these were wrong.
I really wish clang-format comments could be a macro instead of comment.
Adds an explicit test for verifying nested buffers.
Also remove
-Werror=shadowas I think that is too aggressive to be an error. Its common to have nested scopes reuse the same variable name on purpose.