Skip to content

ARROW-1600: [C++] Add Buffer constructor that wraps std::string #1147

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.

Sign up for GitHub

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

Closed
wesm wants to merge 2 commits into from
Closed

ARROW-1600: [C++] Add Buffer constructor that wraps std::string #1147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
644bf2b
Add Buffer ctor that wraps std::string
wesm Sep 29, 2017
f60f502
Remove TestBuffer fixture
wesm Sep 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions cpp/src/arrow/buffer-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@ using std::string;

namespace arrow {

class TestBuffer : public ::testing::Test {};

TEST_F(TestBuffer, IsMutableFlag) {
TEST(TestBuffer, IsMutableFlag) {
Buffer buf(nullptr, 0);

ASSERT_FALSE(buf.is_mutable());
Expand All @@ -46,7 +44,15 @@ TEST_F(TestBuffer, IsMutableFlag) {
ASSERT_TRUE(pbuf.is_mutable());
}

TEST_F(TestBuffer, Resize) {
TEST(TestBuffer, FromStdString) {
std::string val = "hello, world";

Buffer buf(val);
ASSERT_EQ(0, memcmp(buf.data(), val.c_str(), val.size()));
ASSERT_EQ(static_cast<int64_t>(val.size()), buf.size());
}

TEST(TestBuffer, Resize) {
PoolBuffer buf;

ASSERT_EQ(0, buf.size());
Expand All @@ -69,7 +75,7 @@ TEST_F(TestBuffer, Resize) {
ASSERT_EQ(128, buf.capacity());
}

TEST_F(TestBuffer, TypedResize) {
TEST(TestBuffer, TypedResize) {
PoolBuffer buf;

ASSERT_EQ(0, buf.size());
Expand All @@ -88,7 +94,7 @@ TEST_F(TestBuffer, TypedResize) {
ASSERT_EQ(832, buf.capacity());
}

TEST_F(TestBuffer, ResizeOOM) {
TEST(TestBuffer, ResizeOOM) {
// This test doesn't play nice with AddressSanitizer
#ifndef ADDRESS_SANITIZER
// realloc fails, even though there may be no explicit limit
Expand All @@ -99,7 +105,7 @@ TEST_F(TestBuffer, ResizeOOM) {
#endif
}

TEST_F(TestBuffer, EqualsWithSameContent) {
TEST(TestBuffer, EqualsWithSameContent) {
MemoryPool* pool = default_memory_pool();
const int32_t bufferSize = 128 * 1024;
uint8_t* rawBuffer1;
Expand All @@ -123,7 +129,7 @@ TEST_F(TestBuffer, EqualsWithSameContent) {
pool->Free(rawBuffer3, bufferSize);
}

TEST_F(TestBuffer, EqualsWithSameBuffer) {
TEST(TestBuffer, EqualsWithSameBuffer) {
MemoryPool* pool = default_memory_pool();
const int32_t bufferSize = 128 * 1024;
uint8_t* rawBuffer;
Expand All @@ -142,7 +148,7 @@ TEST_F(TestBuffer, EqualsWithSameBuffer) {
pool->Free(rawBuffer, bufferSize);
}

TEST_F(TestBuffer, Copy) {
TEST(TestBuffer, Copy) {
std::string data_str = "some data to copy";

auto data = reinterpret_cast<const uint8_t*>(data_str.c_str());
Expand All @@ -157,7 +163,7 @@ TEST_F(TestBuffer, Copy) {
ASSERT_TRUE(out->Equals(expected));
}

TEST_F(TestBuffer, SliceBuffer) {
TEST(TestBuffer, SliceBuffer) {
std::string data_str = "some data to slice";

auto data = reinterpret_cast<const uint8_t*>(data_str.c_str());
Expand All @@ -171,7 +177,7 @@ TEST_F(TestBuffer, SliceBuffer) {
ASSERT_EQ(2, buf.use_count());
}

TEST_F(TestBuffer, SliceMutableBuffer) {
TEST(TestBuffer, SliceMutableBuffer) {
std::string data_str = "some data to slice";
auto data = reinterpret_cast<const uint8_t*>(data_str.c_str());

Expand Down
44 changes: 32 additions & 12 deletions cpp/src/arrow/buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,25 @@ class MemoryPool;
/// The following invariant is always true: Size < Capacity
class ARROW_EXPORT Buffer {
public:
/// \brief Construct from buffer and size without copying memory
///
/// \param[in] data a memory buffer
/// \param[in] size buffer size
///
/// \note The passed memory must be kept alive through some other means
Copy link
Contributor

@cpcloud cpcloud Sep 29, 2017

Choose a reason for hiding this comment

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

Would ASAN be able to detect this kind of lifetime error?

Copy link
Member Author

@wesm wesm Sep 30, 2017

Choose a reason for hiding this comment

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

I think so, and you would segfault if you tried accessing the memory that had already been freed

Buffer(const uint8_t* data, int64_t size)
: is_mutable_(false), data_(data), size_(size), capacity_(size) {}

/// \brief Construct from std::string without copying memory
///
/// \param[in] data a std::string object
///
/// \note The std::string must stay alive for the lifetime of the Buffer, so
/// temporary rvalue strings must be stored in an lvalue somewhere
explicit Buffer(const std::string& data)
: Buffer(reinterpret_cast<const uint8_t*>(data.c_str()),
static_cast<int64_t>(data.size())) {}

virtual ~Buffer() = default;

/// An offset into data that is owned by another buffer, but we want to be
Expand All @@ -69,6 +85,8 @@ class ARROW_EXPORT Buffer {
/// Return true if both buffers are the same size and contain the same bytes
/// up to the number of compared bytes
bool Equals(const Buffer& other, int64_t nbytes) const;

/// Return true if both buffers are the same size and contain the same bytes
bool Equals(const Buffer& other) const;

/// Copy a section of the buffer into a new Buffer.
Expand Down Expand Up @@ -101,17 +119,6 @@ class ARROW_EXPORT Buffer {
ARROW_DISALLOW_COPY_AND_ASSIGN(Buffer);
};

/// \brief Create Buffer referencing std::string memory
///
/// Warning: string instance must stay alive
///
/// \param str std::string instance
/// \return std::shared_ptr<Buffer>
static inline std::shared_ptr<Buffer> GetBufferFromString(const std::string& str) {
return std::make_shared<Buffer>(reinterpret_cast<const uint8_t*>(str.c_str()),
static_cast<int64_t>(str.size()));
}

/// Construct a view on passed buffer at the indicated offset and length. This
/// function cannot fail and does not error checking (except in debug builds)
static inline std::shared_ptr<Buffer> SliceBuffer(const std::shared_ptr<Buffer>& buffer,
Expand Down Expand Up @@ -331,11 +338,24 @@ Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size,
std::shared_ptr<ResizableBuffer>* out);

#ifndef ARROW_NO_DEPRECATED_API

/// \deprecated Since 0.7.0
ARROW_EXPORT
Status AllocateBuffer(MemoryPool* pool, const int64_t size,
std::shared_ptr<MutableBuffer>* out);
#endif

/// \brief Create Buffer referencing std::string memory
/// \deprecated Since 0.8.0
///
/// Warning: string instance must stay alive
///
/// \param str std::string instance
/// \return std::shared_ptr<Buffer>
static inline std::shared_ptr<Buffer> GetBufferFromString(const std::string& str) {
return std::make_shared<Buffer>(str);
}

#endif // ARROW_NO_DEPRECATED_API

} // namespace arrow

Expand Down
6 changes: 2 additions & 4 deletions cpp/src/arrow/ipc/ipc-json-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,7 @@ TEST(TestJsonFileReadWrite, BasicRoundTrip) {

std::unique_ptr<JsonReader> reader;

auto buffer = std::make_shared<Buffer>(reinterpret_cast<const uint8_t*>(result.c_str()),
static_cast<int>(result.size()));
auto buffer = std::make_shared<Buffer>(result);

ASSERT_OK(JsonReader::Open(buffer, &reader));
ASSERT_TRUE(reader->schema()->Equals(*schema));
Expand Down Expand Up @@ -395,8 +394,7 @@ void CheckRoundtrip(const RecordBatch& batch) {
std::string result;
ASSERT_OK(writer->Finish(&result));

auto buffer = std::make_shared<Buffer>(reinterpret_cast<const uint8_t*>(result.c_str()),
static_cast<int64_t>(result.size()));
auto buffer = std::make_shared<Buffer>(result);

std::unique_ptr<JsonReader> reader;
ASSERT_OK(JsonReader::Open(buffer, &reader));
Expand Down
8 changes: 4 additions & 4 deletions cpp/src/arrow/ipc/ipc-read-write-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,10 @@ TEST(TestMessage, Equals) {
std::string metadata = "foo";
std::string body = "bar";

auto b1 = GetBufferFromString(metadata);
auto b2 = GetBufferFromString(metadata);
auto b3 = GetBufferFromString(body);
auto b4 = GetBufferFromString(body);
auto b1 = std::make_shared<Buffer>(metadata);
auto b2 = std::make_shared<Buffer>(metadata);
auto b3 = std::make_shared<Buffer>(body);
auto b4 = std::make_shared<Buffer>(body);

Message msg1(b1, b3);
Message msg2(b2, b4);
Expand Down