Merge pull request #1551 from m-holger/buffer

Refactor Buffer

Merge pull request #1551 from m-holger/buffer
Refactor Buffer
m-holger · GitHub
2 parents 88a62f78 82c178b0
Showing 4 changed files with 137 additions and 61 deletions
include/qpdf/Buffer.hh
libqpdf/Buffer.cc
libtests/buffer.cc
manual/release-notes.rst
@@ -25,6 +25,7 @@
 #include <cstddef>
 #include <memory>
 #include <string>
+#include <string_view>
 class Buffer
 {
@@ -66,11 +67,34 @@ class Buffer
     QPDF_DLL
     Buffer copy() const;
+    // Move the content of the Buffer. After calling this method, the Buffer will be empty if the
+    // buffer owns its memory. Otherwise, the Buffer will be unchanged.
+    QPDF_DLL
+    std::string move();
+
+    // Return a string_view to the data.
+    QPDF_DLL
+    std::string_view view() const;
+
+    // Return a pointer to the data. NB: Unlike getBuffer, this method returns a valid pointer even
+    // if the Buffer is empty.
+    QPDF_DLL
+    char const* data() const;
+
+    // Return a pointer to the data. NB: Unlike getBuffer, this method returns a valid pointer even
+    // if the Buffer is empty.
+    QPDF_DLL
+    char* data();
+
+    QPDF_DLL
+    bool empty() const;
+
+    QPDF_DLL
+    size_t size() const;
+
   private:
     class Members;
-    void copy(Buffer const&);
-
     std::unique_ptr<Members> m;
 };
@@ -2,75 +2,55 @@
 #include <qpdf/Buffer.hh>
-#include <cstring>
-
 class Buffer::Members
 {
     friend class Buffer;
   public:
-    ~Members();
-
-  private:
-    Members(size_t size, unsigned char* buf, bool own_memory);
-    Members(std::string&& content);
+    Members() = default;
+    // Constructor for Buffers that don't own the memory.
+    Members(size_t size, char* buf) :
+        size(size),
+        buf(buf)
+    {
+    }
+    Members(std::string&& content) :
+        str(std::move(content)),
+        size(str.size()),
+        buf(str.data())
+    {
+    }
     Members(Members const&) = delete;
+    ~Members() = default;
+  private:
     std::string str;
-    bool own_memory;
     size_t size;
-    unsigned char* buf;
+    char* buf;
 };
-Buffer::Members::Members(size_t size, unsigned char* buf, bool own_memory) :
-    own_memory(own_memory),
-    size(size),
-    buf(nullptr)
-{
-    if (own_memory) {
-        this->buf = (size ? new unsigned char[size] : nullptr);
-    } else {
-        this->buf = buf;
-    }
-}
-
-Buffer::Members::Members(std::string&& content) :
-    str(std::move(content)),
-    own_memory(false),
-    size(str.size()),
-    buf(reinterpret_cast<unsigned char*>(str.data()))
-{
-}
-
-Buffer::Members::~Members()
-{
-    if (this->own_memory) {
-        delete[] this->buf;
-    }
-}
-
 Buffer::Buffer() :
-    m(new Members(0, nullptr, true))
+    m(std::make_unique<Members>())
 {
 }
 Buffer::Buffer(size_t size) :
-    m(new Members(size, nullptr, true))
+    m(std::make_unique<Members>(std::string(size, '\0')))
 {
 }
 Buffer::Buffer(std::string&& content) :
-    m(new Members(std::move(content)))
+    m(std::make_unique<Members>(std::move(content)))
 {
 }
 Buffer::Buffer(unsigned char* buf, size_t size) :
-    m(new Members(size, buf, false))
+    m(std::make_unique<Members>(size, reinterpret_cast<char*>(buf)))
 {
 }
 Buffer::Buffer(std::string& content) :
-    m(new Members(content.size(), reinterpret_cast<unsigned char*>(content.data()), false))
+    m(std::make_unique<Members>(content.size(), content.data()))
 {
 }
@@ -88,17 +68,6 @@ Buffer::operator=(Buffer&amp;&amp; rhs) noexcept
 Buffer::~Buffer() = default;
-void
-Buffer::copy(Buffer const& rhs)
-{
-    if (this != &rhs) {
-        m = std::unique_ptr<Members>(new Members(rhs.m->size, nullptr, true));
-        if (m->size) {
-            memcpy(m->buf, rhs.m->buf, m->size);
-        }
-    }
-}
-
 size_t
 Buffer::getSize() const
 {
@@ -108,21 +77,67 @@ Buffer::getSize() const
 unsigned char const*
 Buffer::getBuffer() const
 {
-    return m->buf;
+    return reinterpret_cast<unsigned char*>(m->buf);
 }
 unsigned char*
 Buffer::getBuffer()
 {
-    return m->buf;
+    return reinterpret_cast<unsigned char*>(m->buf);
 }
 Buffer
 Buffer::copy() const
 {
-    auto result = Buffer(m->size);
-    if (m->size) {
-        memcpy(result.m->buf, m->buf, m->size);
+    if (m->size == 0) {
+        return {};
+    }
+    return {std::string(m->buf, m->size)};
+}
+
+std::string
+Buffer::move()
+{
+    if (m->size == 0) {
+        return {};
     }
-    return result;
+    if (!m->str.empty()) {
+        m->size = 0;
+        m->buf = nullptr;
+        return std::move(m->str);
+    }
+    return {m->buf, m->size};
+}
+
+std::string_view
+Buffer::view() const
+{
+    if (!m->buf) {
+        return {};
+    }
+    return {m->buf, m->size};
+}
+
+char const*
+Buffer::data() const
+{
+    return m->buf ? m->buf : m->str.data();
+}
+
+char*
+Buffer::data()
+{
+    return m->buf ? m->buf : m->str.data();
+}
+
+bool
+Buffer::empty() const
+{
+    return m->size == 0;
+}
+
+size_t
+Buffer::size() const
+{
+    return m->size;
 }
@@ -35,6 +35,37 @@ main()
     assert(bc2p[0] == 'R');
     assert(bc2p[1] == 'W');
+    // Test move, view, data, empty and size methods
+    assert(bc1.view() == "RW");
+    assert(std::string_view(bc1.data(), bc1.getSize()) == "RW");
+    *bc1.data() = 'Z';
+    assert(!bc1.empty());
+    assert(bc1.size() == 2);
+    auto s1 = bc1.move();
+    assert(bc1.empty());
+    assert(bc1.size() == 0);
+    assert(!bc1.getBuffer());
+    assert(bc1.getSize() == 0);
+    assert(s1 == "ZW");
+    assert(bc1.view().empty());
+    assert(bc1.data());
+    assert(Buffer(s1).move() == "ZW");
+    assert(s1 == "ZW");
+
+    // Test const methods
+    const Buffer cb(s1);
+    assert(*cb.data() == 'Z');
+    assert(*(cb.getBuffer() + 1) == 'W');
+
+    // Test constructors
+    assert(Buffer().empty());
+    assert(Buffer().copy().empty());
+    assert(!Buffer().getBuffer());
+    assert(Buffer().data());
+    assert(Buffer().move().empty());
+    assert(Buffer(s1).size() == 2);
+    assert(*Buffer(s1).data() == 'Z');
+
     // Test Buffer(std:string&&)
     Buffer bc3("QW");
     unsigned char* bc3p = bc3.getBuffer();
@@ -38,9 +38,15 @@ more detail.
       overhead of repeatedly validating the underlying document structure
       and/or building internal caches. If the underlying document structure
       is directly modified (without the use of DocumentHelpers), the
-      ``validate`` methods revalidates the structure and resynchronizes any
+      ``validate`` methods revalidate the structure and resynchronize any
       internal caches.
+    - Add new ``Buffer`` methods ``move``, ``view``, ``data``, ``size`` and
+      ``empty``. The new methods present the ``Buffer`` as a ``char`` (rather
+      than ``unsigned char``) container and facilitate the efficient moving
+      of its content into a `std::string``.
+
+
   - CLI Enhancements
    - Disallow option :qpdf:ref:`--deterministic-id` to be used together