Skip to content
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

[C++] Add convenience method to construct Buffer from a string that owns its memory #17634

Closed
asfimport opened this issue Sep 29, 2017 · 6 comments

Comments

@asfimport
Copy link

The memory would need to be allocated from a memory pool / buffer allocator

Reporter: Wes McKinney / @wesm
Assignee: Panchen Xue / @xuepanchen

PRs and other links:

Note: This issue was originally created as ARROW-1623. Please see the migration documentation for further details.

@asfimport
Copy link
Author

Panchen Xue / @xuepanchen:
Is this for base class Buffer or derived PoolBuffer?

@asfimport
Copy link
Author

Uwe Korn / @xhochy:
This is for the base class Buffer. PoolBuffer owns it's memory directly where the method to be implemented here will use memory that is owned by a string instance.

@asfimport
Copy link
Author

Wes McKinney / @wesm:
The API would be something like

Status Buffer::FromString(const std::string& value, MemoryPool* pool, std::shared_ptr<Buffer>* out)

@asfimport
Copy link
Author

Panchen Xue / @xuepanchen:
My understanding is:

Status Buffer::FromString(const std::string& data, MemoryPool* pool, std::shared_ptr<Buffer>* out) const {
  auto new_buffer = std::make_shared<PoolBuffer>(pool);
  auto size = static_cast<int64_t>(data.size());
  auto str = reinterpret_cast<const uint8_t*>(data.c_str());

  RETURN_NOT_OK(new_buffer->Resize(size));
  std::memcpy(new_buffer->mutable_data(), str, size);

  *out = new_buffer;
  return Status::OK();
}

Is it correct? Or shall we change the calling object?

@asfimport
Copy link
Author

Wes McKinney / @wesm:
Yes, a static member function would be fine. You can use AllocateBuffer to save a line of code

@asfimport
Copy link
Author

Wes McKinney / @wesm:
Issue resolved by pull request 1518
#1518

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant