buffer zero alloc

src/eckit/io/Buffer.cc

@@ -8,6 +8,7 @@
  * does it submit to any jurisdiction.
  */
 
+#include <algorithm>
 #include <cstring>
 
 #include "eckit/exception/Exceptions.h"
@@ -19,31 +20,29 @@ namespace eckit {
 
 namespace {
 
-static char* allocate(size_t size) {
-    return new char[size];
+static char* allocate(const size_t size) {
+    return size == 0 ? nullptr : new char[size];
 }
 
-static void deallocate(char* buffer) {
+static void deallocate(char*& buffer) {
     delete[] buffer;
+    buffer = nullptr;
 }
 
 }  // namespace
 
 //----------------------------------------------------------------------------------------------------------------------
 
-Buffer::Buffer(size_t size) :
-    buffer_{nullptr}, size_{size} {
+Buffer::Buffer(const size_t size): size_ {size} {
     create();
 }
 
-Buffer::Buffer(const void* p, size_t len) :
-    buffer_{nullptr}, size_{len} {
+Buffer::Buffer(const void* p, size_t len): size_ {len} {
     create();
     copy(p, len);
 }
 
-Buffer::Buffer(const std::string& s) :
-    buffer_{nullptr}, size_{s.length() + 1} {
+Buffer::Buffer(const std::string& s): size_ {s.length() + 1} {
     create();
     copy(s);
 }
@@ -85,26 +84,22 @@ void Buffer::create() {
 }
 
 void Buffer::destroy() {
-    if (buffer_) {
-        deallocate(buffer_);
-        buffer_ = nullptr;
-        size_   = 0;
-    }
+    deallocate(buffer_);
+    size_ = 0;
 }
 
 void Buffer::copy(const std::string& s) {
-    ASSERT(buffer_);
-    ::strncpy(buffer_, s.c_str(), std::min(size_, s.size() + 1));
+    if (buffer_) { ::strncpy(buffer_, s.c_str(), std::min(size_, s.size() + 1)); }
 }
 
-void Buffer::copy(const void* p, size_t size, size_t pos) {
-    ASSERT(buffer_ && size_ >= pos + size);
-    if (size) {
+void Buffer::copy(const void* p, const size_t size, const size_t pos) {
+    ASSERT(size_ >= pos + size);
+    if (buffer_ && size > 0) {
         ::memcpy(buffer_ + pos, p, size);
     }
 }
 
-void Buffer::resize(size_t size, bool preserveData) {
+void Buffer::resize(const size_t size, const bool preserveData) {
     if (size != size_) {
         if (preserveData) {
             char* newbuffer = allocate(size);

src/eckit/io/Buffer.h

@@ -26,11 +26,20 @@ namespace eckit {
 
 class Buffer : private NonCopyable {
 public:  // methods
+    /// Creates a buffer with 'size' many bytes.
+    /// @param size in bytes of the buffer.
     explicit Buffer(size_t size = 0);
+
+    /// Creates a buffer from the string s.
+    /// NOTE: the buffer will contain the string as a zero terminated c-string.
+    /// I.e. the resulting size of buffer is s.size() + 1
+    /// @param s, initial content of buffer, including null-terminator
     explicit Buffer(const std::string& s);
 
-    /// Allocate and copy memory of given length in bytes
-    Buffer(const void*, size_t len);
+    /// Creates buffer with initial content
+    /// @param src to copy bytes from
+    /// @param len of data
+    Buffer(const void* src, size_t len);
 
     /// Move constructor. Note that rhs is not guaranteed to be valid!
     Buffer(Buffer&& rhs) noexcept;
@@ -49,14 +58,21 @@ class Buffer : private NonCopyable {
     void* data() { return buffer_; }
     const void* data() const { return buffer_; }
 
-    /// @return allocated size
+    /// Returns size of the buffer in bytes
+    /// Note: The actual allocation held by this buffer may be larger
+    /// if the buffer has been resized with 'preserveData' set to a smaller size than before.
+    /// @return buffer size in bytes
     size_t size() const { return size_; }
 
     /// Zero content of buffer
     void zero();
 
-    /// @post Invalidates contents of buffer
-    void resize(size_t, bool preserveData = false);
+    /// Resizes the buffer to newSize.
+    /// When 'preserveData' is set:
+    /// - Buffer will still hold the data contained before resize() was called.
+    /// - If 'newSize' is smaller than the current 'size' the data will be truncated.
+    /// With unset 'preserveData' the contents of the buffer will be discarded.
+    void resize(size_t newSize, bool preserveData = false);
 
     /// Copy data of given size (bytes) into buffer at given position
     /// @pre Buffer must have sufficient size
@@ -65,7 +81,9 @@ class Buffer : private NonCopyable {
 protected:  // methods
     void create();
     void destroy();
-
+    /// Copies contents of s into the buffer including a null-terminator at the end.
+    /// This copy operation truncates s if the buffer is not big enough.
+    /// @param s, string to be copied including null-terminator.
     void copy(const std::string& s);
 
 private:  // members

tests/io/test_buffer.cc

@@ -4,6 +4,7 @@
 #include <ctime>
 
 #include <algorithm>
+#include <memory>
 
 #include "eckit/io/Buffer.h"
 #include "eckit/testing/Test.h"
@@ -18,21 +19,22 @@ namespace eckit::test {
 
 //----------------------------------------------------------------------------------------------------------------------
 
-unsigned char* random_bytestream(size_t sz) {
-    ::srand((unsigned int)::time(0));
+unsigned char* random_bytestream(const size_t sz) {
+    ::srand(static_cast<unsigned int>(::time(nullptr)));
     unsigned char* stream = static_cast<unsigned char*>(::malloc(sz));
     auto rnd              = []() -> unsigned char { return ::rand(); };
     std::generate(stream, stream + sz, rnd);
     // std::for_each(stream, stream + sz, [](unsigned char c) { std::cout << c ; });
     return stream;
 }
 
-const char* msg = "Once upon a midnight dreary";
+constexpr auto msg = "Once upon a midnight dreary";
+constexpr auto msgLen = std::char_traits<char>::length(msg);
 
 CASE("Test eckit Buffer default constructor") {
     Buffer buf;
     EXPECT(buf.size() == 0);
-    EXPECT(buf.data() != nullptr);
+    EXPECT(buf.data() == nullptr);
 }
 
 CASE("Test eckit Buffer constructor 1") {
@@ -68,7 +70,8 @@ CASE("Test eckit Buffer move constructor") {
     const char* out = buf2;
     EXPECT(std::strcmp(msg, out) == 0);
 
-    EXPECT(static_cast<const char*>(buf1) == nullptr && buf1.size() == 0);
+    EXPECT(static_cast<const char*>(buf1) == nullptr);
+    EXPECT_EQUAL(buf1.size(), 0);
 }
 
 CASE("Test eckit Buffer move assignment") {
@@ -81,8 +84,8 @@ CASE("Test eckit Buffer move assignment") {
     const char* out = buf2;
     EXPECT(std::strcmp(msg, out) == 0);
 
-    // EXPECT(static_cast<const char*>(buf1) == nullptr)
-    EXPECT(buf1.size() == 0);
+    EXPECT(static_cast<const char*>(buf1) == nullptr);
+    EXPECT_EQUAL(buf1.size(), 0);
 }
 
 // This includes a self-move but is legitimate, if pointless, so it should be tested
@@ -114,14 +117,17 @@ CASE("Test eckit Buffer Zero") {
 
 // NOTE: resize allocates a new buffer whenever the new size is different -- this is inefficient
 CASE("Test eckit Buffer resize") {
-    const size_t sz = std::strlen(msg) + 1;
+    constexpr auto sz = msgLen + 1;
     Buffer buf;
     EXPECT(buf.size() == 0);
 
     EXPECT_THROWS(buf.copy(msg, sz));
 
     EXPECT_NO_THROW(buf.resize(sz));
+    EXPECT_EQUAL(buf.size(), sz);
+
     EXPECT_NO_THROW(buf.copy(msg, sz));
+    EXPECT_EQUAL(std::strncmp(msg, static_cast<const char*>(buf), sz), 0);
 
     size_t newSize = 41;
     buf.resize(newSize, true);
@@ -139,6 +145,34 @@ CASE("Test eckit Buffer resize") {
     newSize = 41;
     buf.resize(newSize, false);
     EXPECT(buf.size() == newSize);
+
+    buf.resize(0, false);
+    EXPECT_EQUAL(buf.size(), 0);
+    EXPECT(buf.data() == nullptr);
+
+    buf.resize(0, true);
+    EXPECT_EQUAL(buf.size(), 0);
+    EXPECT(buf.data() == nullptr);
+}
+
+CASE("Test copy from temp buffer") {
+    struct BufferTester {
+        explicit BufferTester(const Buffer& buffer = Buffer {0}): buf_ {buffer, buffer.size()} { }
+
+        Buffer buf_;
+    };
+
+    std::unique_ptr<BufferTester> tester;
+
+    // empty
+    EXPECT_NO_THROW(tester = std::make_unique<BufferTester>());
+    EXPECT(tester->buf_.data() == nullptr);
+    EXPECT_EQUAL(tester->buf_.size(), 0);
+
+    // non-empty
+    EXPECT_NO_THROW(tester = std::make_unique<BufferTester>(Buffer {msg, msgLen}));
+    EXPECT_EQUAL(std::strncmp(msg, static_cast<const char*>(tester->buf_), msgLen), 0);
+    EXPECT_EQUAL(tester->buf_.size(), msgLen);
 }
 
 CASE("Test copying and construction from of std::string") {