Skip to content

Comments

DetachedBuffer cannot be rewrapped#4885

Merged
aardappel merged 6 commits intogoogle:masterfrom
gabyx:master
Aug 24, 2018
Merged

DetachedBuffer cannot be rewrapped#4885
aardappel merged 6 commits intogoogle:masterfrom
gabyx:master

Conversation

@gabyx
Copy link
Contributor

@gabyx gabyx commented Aug 17, 2018

Pull request for Issue #4848.

Added a DetachedBufferRaw for external rewrapping.

@gabyx
Copy link
Contributor Author

gabyx commented Aug 18, 2018

auto dBufferRaw = builder.ReleaseRaw();

in #4885 : I tried to have this release functionality discused in #4848 : the problem I stumbled upon is:
FlatBufferBuilder::ReleaseRaw's semantic is kind of strange.
First, if the user relies on the DefaultAllocator, he is responsible of deleting the memory also over this allocator, so dBufferRaw.allocator() == nullptr means deleting with flatbuffers:DefaultAllocator.

Second, there is this bool, own_allocator_ which is kind of troublesome...,
What should dBufferRaw do? I decided to do exactly nothing:

  • DetachedBufferRaw does not at all manage the memory (exactly the behavior for rewrapping somewhere else..)
  • no memory management for the allocated memory and also not for theallocator() itself.
  • it should only be movable, since we should not keep copies lying around (move should nullify the class, not yet done...)

Maybe we could in the future remove the DetachedBuffer class which relies on memory management behavior, or simply leave it in, since it might be useful for some users...

Thanks for your help.

/// case `allocator() == nullptr` (where the DefaultAllocator has been used!).
/// The user is also responsible to delete the allocator `allocator()` if it
/// is owned `allocator_owned()`.
class DetachedBufferRaw {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets not have yet another class for this. The idea was a simple function that just gives you a raw pointer back, + size/offset

Copy link
Contributor Author

@gabyx gabyx Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also happy with such an approach. without std::tuple. will change it...

@gabyx
Copy link
Contributor Author

gabyx commented Aug 20, 2018

do you rather prefer a return struct or a return by input arguments? or a std::pair<std::pair<uint8_t*, size_t>, size_t>.

I would define a return struct...

@aardappel
Copy link
Collaborator

I would just keep it super simple, e.g. uint8_t *DetachRaw(size_t *size, size_t *offset). This is very low-level access, so the API doesn't need to be pretty.

@gabyx
Copy link
Contributor Author

gabyx commented Aug 20, 2018

:) ok. will adapt to this intention. super simple is ok. super ugly not. will make arg references :). thx

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot
Copy link

CLAs look good, thanks!

@gabyx
Copy link
Contributor Author

gabyx commented Aug 21, 2018

Requesting review, for my changes :-) Thanks =)


// Relinquish the pointer to the caller.
uint8_t *release_raw(size_t &size, size_t &offset) {
uint8_t *buf = buf_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto

size = reserved_;
offset = static_cast<size_t>(cur_ - buf_);

if (own_allocator_ && allocator_) { delete allocator_; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe put these 3 lines in a clear_allocator and also call it from the destructor?

@gabyx
Copy link
Contributor Author

gabyx commented Aug 23, 2018 via email

}

void clear_allocator()
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ on prev line, same below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, I couldnt somehow use clang-format -> kind of messes up everything, when I apply it... (i use clang 6.0 maybe thats why...

@aardappel
Copy link
Collaborator

7/include/flatbuffers/flatbuffers.h:611:54: error: declaration of ‘size’ shadows a member of 'this' [-Werror=shadow]
   uint8_t *release_raw(size_t &size, size_t &offset) {

@gabyx
Copy link
Contributor Author

gabyx commented Aug 24, 2018

There is no member size ? Except the member function size(). Is it that? Strange...
why should there be a shadowing?

@aardappel
Copy link
Collaborator

see travis output

@aardappel
Copy link
Collaborator

Looks good, thanks!

@aardappel aardappel merged commit 99acd0b into google:master Aug 24, 2018
zchee pushed a commit to zchee/flatbuffers that referenced this pull request Feb 14, 2019
* Simple ReleaseRaw implemented

* [doc]

* clear_buffer and clear_allocator introduced

* auto

* typos

* rename because of -Werror=shadow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants