Merge pull request #193 from jpcima/buffer-movable

Do some cleanup in the Buffer class

src/sfizz/AudioBuffer.h

@@ -64,12 +64,12 @@ public:
      * @return true if the resize worked
      * @return false otherwise
      */
-    bool resize(size_t newSize)
+    bool resize(size_t newSize, std::nothrow_t) noexcept
     {
         bool returnedOK = true;
 
         for (size_t i = 0; i < numChannels; ++i)
-            returnedOK &= buffers[i]->resize(newSize);
+            returnedOK &= buffers[i]->resize(newSize, std::nothrow);
 
         if (returnedOK)
             numFrames = newSize;
@@ -77,6 +77,19 @@ public:
         return returnedOK;
     }
 
+    /**
+     * @brief Resizes all the underlying buffers to a new size.
+     *
+     * @param newSize
+     */
+    void resize(size_t newSize)
+    {
+        for (size_t i = 0; i < numChannels; ++i)
+            buffers[i]->resize(newSize);
+
+        numFrames = newSize;
+    }
+
     /**
      * @brief Return an iterator to a specific channel with a non-const type.
      *

src/sfizz/Buffer.h

@@ -45,8 +45,18 @@ namespace sfz
 class BufferCounter
 {
 public:
-    BufferCounter() = default;
+    /**
-    ~BufferCounter()
+     * @brief      Return the buffer counter object.
+     */
+    static BufferCounter& counter() noexcept
+    {
+        static BufferCounter counter;
+        return counter;
+    }
+
+protected:
+    BufferCounter() noexcept = default;
+    ~BufferCounter() noexcept
     {
 #ifndef NDEBUG
         std::cout << "Remaining buffers on destruction: " << numBuffers << '\n';
@@ -54,41 +64,42 @@ public:
 #endif
     }
 
-    void newBuffer(int size)
+public:
+    void newBuffer(int size) noexcept
     {
         numBuffers++;
         bytes.fetch_add(size);
     }
 
-    void bufferResized(int oldSize, int newSize)
+    void bufferResized(int oldSize, int newSize) noexcept
     {
         bytes.fetch_add(newSize);
         bytes.fetch_sub(oldSize);
     }
 
-    void bufferDeleted(int size)
+    void bufferDeleted(int size) noexcept
     {
         numBuffers--;
         bytes.fetch_sub(size);
     }
 
-    void bufferDeleted(size_t size)
+    void bufferDeleted(size_t size) noexcept
     {
         bufferDeleted(static_cast<int>(size));
     }
 
-    void bufferResized(size_t oldSize, size_t newSize)
+    void bufferResized(size_t oldSize, size_t newSize) noexcept
     {
         bufferResized(static_cast<int>(oldSize), static_cast<int>(newSize));
     }
 
-    void newBuffer(size_t size)
+    void newBuffer(size_t size) noexcept
     {
         newBuffer(static_cast<int>(size));
     }
 
-    int getNumBuffers() { return numBuffers; }
+    int getNumBuffers() const noexcept { return numBuffers; }
-    int getTotalBytes() { return bytes; }
+    int getTotalBytes() const noexcept { return bytes; }
 private:
     std::atomic<int> numBuffers { 0 };
     std::atomic<int> bytes { 0 };
@@ -128,9 +139,8 @@ public:
      * @brief Construct a new Buffer object that is empty
      *
      */
-    Buffer()
+    Buffer() noexcept
     {
-        counter().newBuffer(0);
     }
 
     /**
@@ -138,9 +148,8 @@ public:
      *
      * @param size
      */
-    Buffer(size_t size)
+    explicit Buffer(size_t size)
     {
-        counter().newBuffer(0);
         resize(size);
     }
 
@@ -152,7 +161,7 @@ public:
      * @return true if allocation succeeded
      * @return false otherwise
      */
-    bool resize(size_t newSize)
+    bool resize(size_t newSize, std::nothrow_t) noexcept
     {
         if (newSize == 0) {
             clear();
@@ -160,19 +169,24 @@ public:
         }
 
         auto tempSize = newSize + 2 * AlignmentMask; // To ensure that we have leeway at the beginning and at the end
-        auto* newData = paddedData != nullptr ? std::realloc(paddedData, tempSize * sizeof(value_type)) : std::malloc(tempSize * sizeof(value_type));
+        auto* newData = std::realloc(paddedData.get(), tempSize * sizeof(value_type));
         if (newData == nullptr) {
             return false;
         }
 
-        counter().bufferResized(largerSize * sizeof(value_type), tempSize * sizeof(value_type));
+        if (largerSize > 0)
+            counter().bufferResized(largerSize * sizeof(value_type), tempSize * sizeof(value_type));
+        else
+            counter().newBuffer(tempSize * sizeof(value_type));
+
         largerSize = tempSize;
         alignedSize = newSize;
-        paddedData = static_cast<pointer>(newData);
+        paddedData.release(); // realloc has invalidated the old pointer
+        paddedData.reset(static_cast<pointer>(newData));
         normalData = static_cast<pointer>(align(Alignment, alignedSize, newData, tempSize));
         normalEnd = normalData + alignedSize;
-		auto endMisalignment = (alignedSize & TypeAlignmentMask);
+        auto endMisalignment = (alignedSize & TypeAlignmentMask);
-		if (endMisalignment != 0)
+        if (endMisalignment != 0)
             _alignedEnd = normalEnd + Alignment - endMisalignment;
         else
             _alignedEnd = normalEnd;
@@ -180,24 +194,40 @@ public:
         return true;
     }
 
+    /**
+     * @brief Resizes the buffer. Given that std::realloc may return either the same pointer
+     * or a new one, you need to account for both cases in the code if you are using deep pointers.
+     *
+     * @param newSize the new buffer size in bytes
+     */
+    void resize(size_t newSize)
+    {
+        if (!resize(newSize, std::nothrow))
+            throw std::bad_alloc();
+    }
+
     /**
      * @brief Clear the buffers and frees the underlying memory
      *
      */
-    void clear()
+    void clear() noexcept
     {
-        counter().bufferResized(largerSize * sizeof(value_type), static_cast<size_t>(0));
+        if (largerSize > 0)
-        largerSize = 0;
+            counter().bufferDeleted(largerSize * sizeof(value_type));
-        alignedSize = 0;
+        _clear();
-        std::free(paddedData);
-        normalData = nullptr;
-        normalEnd = nullptr;
-        _alignedEnd = nullptr;
     }
-    ~Buffer()
+    ~Buffer() noexcept
     {
-        counter().bufferDeleted(largerSize * sizeof(value_type));
+        if (largerSize > 0)
-        std::free(paddedData);
+            counter().bufferDeleted(largerSize * sizeof(value_type));
+    }
+
+    /**
+     * @brief Get the size of the larger block which is actually allocated
+     */
+    size_t allocationSize() const noexcept
+    {
+        return largerSize;
     }
 
     /**
@@ -207,10 +237,8 @@ public:
      */
     Buffer(const Buffer<Type>& other)
     {
-        counter().newBuffer(0);
+        resize(other.size());
-        if (resize(other.size())) {
+        std::memcpy(this->data(), other.data(), other.size() * sizeof(value_type));
-            std::memcpy(this->data(), other.data(), other.size() * sizeof(value_type));
-        }
     }
 
     /**
@@ -218,45 +246,43 @@ public:
      *
      * @param other
      */
-    Buffer(Buffer<Type>&& other) = delete;
+    Buffer(Buffer<Type>&& other) noexcept
-    // {
+        : largerSize(other.largerSize),
-    //     if (this != &other) {
+          alignedSize(other.alignedSize),
-    //         counter().bufferDeleted(largerSize * sizeof(value_type));
+          normalData(other.normalData),
-    //         std::free(paddedData);
+          paddedData(std::move(other.paddedData)),
-    //         largerSize = std::exchange(other.largerSize, 0);
+          normalEnd(other.normalEnd),
-    //         alignedSize = std::exchange(other.alignedSize, 0);
+          _alignedEnd(other._alignedEnd)
-    //         paddedData = std::exchange(other.paddedData, nullptr);
+    {
-    //         normalData = std::exchange(other.normalData, nullptr);
+        other._clear();
-    //         normalEnd = std::exchange(other.normalEnd, nullptr);
+    }
-    //         _alignedEnd = std::exchange(other._alignedEnd, nullptr);
-    //     }
-    // }
 
     Buffer<Type>& operator=(const Buffer<Type>& other)
     {
         if (this != &other) {
-            if (resize(other.size()))
+            resize(other.size());
-                std::memcpy(this->data(), other.data(), other.size() * sizeof(value_type));
+            std::memcpy(this->data(), other.data(), other.size() * sizeof(value_type));
         }
         return *this;
     }
 
-    Buffer<Type>& operator=(Buffer<Type>&& other) = delete;
+    Buffer<Type>& operator=(Buffer<Type>&& other) noexcept
-    // {
+    {
-    //     if (this != &other) {
+        if (this != &other) {
-    //         counter().bufferDeleted(largerSize * sizeof(value_type));
+            if (largerSize > 0)
-    //         std::free(paddedData);
+                counter().bufferDeleted(largerSize * sizeof(value_type));
-    //         largerSize = std::exchange(other.largerSize, 0);
+            largerSize = other.largerSize;
-    //         alignedSize = std::exchange(other.alignedSize, 0);
+            alignedSize = other.alignedSize;
-    //         paddedData = std::exchange(other.paddedData, nullptr);
+            normalData = other.normalData;
-    //         normalData = std::exchange(other.normalData, nullptr);
+            paddedData = std::move(other.paddedData);
-    //         normalEnd = std::exchange(other.normalEnd, nullptr);
+            normalEnd = other.normalEnd;
-    //         _alignedEnd = std::exchange(other._alignedEnd, nullptr);
+            _alignedEnd = other._alignedEnd;
-    //     }
+            other._clear();
-    //     return *this;
+        }
-    // }
+        return *this;
+    }
 
-    Type& operator[](size_t idx) { return *(normalData + idx); }
+    Type& operator[](size_t idx) noexcept { return *(normalData + idx); }
     constexpr pointer data() const noexcept { return normalData; }
     constexpr size_type size() const noexcept { return alignedSize; }
     constexpr bool empty() const noexcept { return alignedSize == 0; }
@@ -264,19 +290,27 @@ public:
     constexpr iterator end() const noexcept { return normalEnd; }
     constexpr pointer alignedEnd() const noexcept { return _alignedEnd; }
 
-
     /**
      * @brief      Return the buffer counter object.
      *
-     *             TODO: In C++ 17 all of this can be static inline.
-     *
      * @return     The buffer counter on which you can call various methods.
      */
-    static BufferCounter& counter()
+    static BufferCounter& counter() noexcept
     {
-        static BufferCounter counter;
+        return BufferCounter::counter();
-        return counter;
     }
+
+private:
+    void _clear()
+    {
+        largerSize = 0;
+        alignedSize = 0;
+        paddedData.reset();
+        normalData = nullptr;
+        normalEnd = nullptr;
+        _alignedEnd = nullptr;
+    }
+
 private:
     static constexpr int AlignmentMask { Alignment - 1 };
     static constexpr int TypeAlignment { Alignment / sizeof(value_type) };
@@ -295,10 +329,14 @@ private:
         return ptr = reinterpret_cast< void * >( aligned );
     }
 
+    struct deleter {
+        void operator()(void *p) const noexcept { std::free(p); }
+    };
+
     size_type largerSize { 0 };
     size_type alignedSize { 0 };
     pointer normalData { nullptr };
-    pointer paddedData { nullptr };
+    std::unique_ptr<value_type[], deleter> paddedData;
     pointer normalEnd { nullptr };
     pointer _alignedEnd { nullptr };
     LEAK_DETECTOR(Buffer);

tests/BufferT.cpp

@@ -70,12 +70,12 @@ TEST_CASE("[Buffer] Resize 10 floats ")
 
     std::fill(buffer.begin(), buffer.end(), 1.0f);
 
-    REQUIRE(buffer.resize(smallSize));
+    REQUIRE(buffer.resize(smallSize, std::nothrow));
     checkBoundaries(buffer, smallSize);
 
     REQUIRE(std::all_of(buffer.begin(), buffer.end(), [](float value) { return value == 1.0f; }));
 
-    REQUIRE(buffer.resize(bigSize));
+    REQUIRE(buffer.resize(bigSize, std::nothrow));
     checkBoundaries(buffer, bigSize);
     for (auto i = 0; i < smallSize; ++i)
         REQUIRE(buffer[i] == 1.0f);
@@ -92,12 +92,12 @@ TEST_CASE("[Buffer] Resize 4096 floats ")
 
     std::fill(buffer.begin(), buffer.end(), 1.0f);
 
-    REQUIRE(buffer.resize(smallSize));
+    REQUIRE(buffer.resize(smallSize, std::nothrow));
     checkBoundaries(buffer, smallSize);
 
     REQUIRE(std::all_of(buffer.begin(), buffer.end(), [](float value) { return value == 1.0f; }));
 
-    REQUIRE(buffer.resize(bigSize));
+    REQUIRE(buffer.resize(bigSize, std::nothrow));
     checkBoundaries(buffer, bigSize);
     for (auto i = 0; i < smallSize; ++i)
         REQUIRE(buffer[i] == 1.0f);
@@ -114,12 +114,12 @@ TEST_CASE("[Buffer] Resize 65536 floats ")
 
     std::fill(buffer.begin(), buffer.end(), 1.0f);
 
-    REQUIRE(buffer.resize(smallSize));
+    REQUIRE(buffer.resize(smallSize, std::nothrow));
     checkBoundaries(buffer, smallSize);
 
     REQUIRE(std::all_of(buffer.begin(), buffer.end(), [](float value) { return value == 1.0f; }));
 
-    REQUIRE(buffer.resize(bigSize));
+    REQUIRE(buffer.resize(bigSize, std::nothrow));
     checkBoundaries(buffer, bigSize);
     for (auto i = 0; i < smallSize; ++i)
         REQUIRE(buffer[i] == 1.0f);
@@ -140,8 +140,64 @@ TEST_CASE("[Buffer] Copy and move")
     checkBoundaries(copyConstructed, baseSize);
     REQUIRE(std::all_of(copyConstructed.begin(), copyConstructed.end(), [](float value) { return value == 1.0f; }));
 
-    // sfz::Buffer<float> moveConstructed { std::move(buffer) };
+    sfz::Buffer<float> moveConstructed { std::move(buffer) };
-    // REQUIRE(buffer.empty());
+    REQUIRE(buffer.empty());
-    // checkBoundaries(moveConstructed, baseSize);
+    checkBoundaries(moveConstructed, baseSize);
-    // REQUIRE(std::all_of(moveConstructed.begin(), moveConstructed.end(), [](auto value) { return value == 1.0f; }));
+    REQUIRE(std::all_of(moveConstructed.begin(), moveConstructed.end(), [](float value) { return value == 1.0f; }));
+}
+
+TEST_CASE("[Buffer] Buffer counter")
+{
+    sfz::BufferCounter& counter = sfz::Buffer<float>::counter();
+
+    // handle the eventuality that the buffer counter does not start at zero
+    const int initialNumBuffers = counter.getNumBuffers();
+    const int initialTotalBytes = counter.getTotalBytes();
+    auto haveNumBuffers = [&](int n) -> bool {
+        return n == counter.getNumBuffers() - initialNumBuffers;
+    };
+    auto haveTotalAllocation = [&](int n) -> bool {
+        return n * static_cast<int>(sizeof(float)) == counter.getTotalBytes() - initialTotalBytes;
+    };
+
+    // create an empty buffer
+    sfz::Buffer<float> b1;
+    REQUIRE(haveNumBuffers(0));
+    REQUIRE(haveTotalAllocation(0));
+
+    // clear an empty buffer
+    b1.clear();
+    REQUIRE(haveNumBuffers(0));
+    REQUIRE(haveTotalAllocation(0));
+
+    // create a sized buffer
+    sfz::Buffer<float> b2(5);
+    REQUIRE(haveNumBuffers(1));
+    REQUIRE(haveTotalAllocation(b2.allocationSize()));
+
+    // resize an empty buffer
+    b1.resize(3);
+    REQUIRE(haveNumBuffers(2));
+    REQUIRE(haveTotalAllocation(b1.allocationSize() + b2.allocationSize()));
+
+    // resize a non-empty buffer
+    b1.resize(7);
+    REQUIRE(haveNumBuffers(2));
+    REQUIRE(haveTotalAllocation(b1.allocationSize() + b2.allocationSize()));
+
+    // clear a non-empty buffer
+    b2.clear();
+    REQUIRE(haveNumBuffers(1));
+    REQUIRE(haveTotalAllocation(b1.allocationSize()));
+
+    // move an empty buffer into a non-empty one
+    b1 = std::move(b2);
+    REQUIRE(haveNumBuffers(0));
+    REQUIRE(haveTotalAllocation(0));
+
+    // move a non-empty buffer into an empty one
+    b1.resize(3);
+    b2 = std::move(b1);
+    REQUIRE(haveNumBuffers(1));
+    REQUIRE(haveTotalAllocation(b2.allocationSize()));
 }