Copilot commented on code in PR #2617:
URL: https://github.com/apache/orc/pull/2617#discussion_r3199294313
##########
c++/src/MemoryPool.cc:
##########
@@ -114,6 +134,18 @@ namespace orc {
memset(buf_, 0, sizeof(T) * currentCapacity_);
}
+ template <class T>
+ void DataBuffer<T>::setData(T* buffer, size_t bufSize) {
+ if (ownBuffer_ && buf_) {
+ static_assert(std::is_trivially_copyable<T>::value,
+ "Only trivially copyable type is supported for DataBuffer
reserve");
+ memoryPool_.free(reinterpret_cast<char*>(buf_));
+ }
+ ownBuffer_ = false;
+ buf_ = buffer;
+ currentSize_ = currentCapacity_ = static_cast<uint64_t>(bufSize /
sizeof(T));
+ }
Review Comment:
`setData` interprets `bufSize` as bytes and silently truncates when
`bufSize` is not a multiple of `sizeof(T)` (because of integer division).
Please validate that `bufSize % sizeof(T) == 0` (assert/throw) or change the
API to take an element count to avoid subtle size/capacity bugs.
##########
c++/test/TestCache.cc:
##########
@@ -26,6 +26,22 @@
namespace orc {
+ class CountingMemoryPool : public MemoryPool {
+ public:
+ uint64_t allocCount = 0;
+ uint64_t freeCount = 0;
+
+ char* malloc(uint64_t size) override {
+ ++allocCount;
+ return static_cast<char*>(std::malloc(size));
+ }
+
+ void free(char* p) override {
+ ++freeCount;
+ std::free(p);
+ }
Review Comment:
`CountingMemoryPool` calls `std::malloc`/`std::free` but this file doesn’t
include `<cstdlib>`. Please add the include (or avoid `std::` and rely on
`::malloc`/`::free`) to prevent build failures depending on indirect includes.
##########
c++/src/MemoryPool.cc:
##########
@@ -52,35 +53,51 @@ namespace orc {
}
template <class T>
- DataBuffer<T>::DataBuffer(MemoryPool& pool, uint64_t newSize)
- : memoryPool_(pool), buf_(nullptr), currentSize_(0), currentCapacity_(0)
{
- reserve(newSize);
- currentSize_ = newSize;
+ DataBuffer<T>::DataBuffer(MemoryPool& pool, uint64_t newSize, bool ownBuf)
+ : memoryPool_(pool),
+ buf_(nullptr),
+ currentSize_(0),
+ currentCapacity_(0),
+ ownBuffer_(ownBuf) {
+ if (ownBuffer_) {
+ reserve(newSize);
+ currentSize_ = newSize;
+ }
}
template <class T>
DataBuffer<T>::DataBuffer(DataBuffer<T>&& buffer) noexcept
: memoryPool_(buffer.memoryPool_),
buf_(buffer.buf_),
currentSize_(buffer.currentSize_),
- currentCapacity_(buffer.currentCapacity_) {
+ currentCapacity_(buffer.currentCapacity_),
+ ownBuffer_(buffer.ownBuffer_) {
buffer.buf_ = nullptr;
buffer.currentSize_ = 0;
buffer.currentCapacity_ = 0;
+ buffer.ownBuffer_ = true;
}
template <class T>
DataBuffer<T>::~DataBuffer() {
+ if (!ownBuffer_) {
+ return;
+ }
for (uint64_t i = currentSize_; i > 0; --i) {
(buf_ + i - 1)->~T();
}
if (buf_) {
+ static_assert(std::is_trivially_copyable<T>::value,
+ "Only trivially copyable type is supported for DataBuffer
reserve");
memoryPool_.free(reinterpret_cast<char*>(buf_));
}
Review Comment:
The `static_assert` message says “supported for DataBuffer reserve”, but
it’s currently enforced in the destructor / `setData` free-path. Consider
moving the `static_assert(std::is_trivially_copyable_v<T>)` into `reserve()`
(and/or at class scope) and updating the message so it matches the actual
constraint being enforced.
##########
c++/include/orc/MemoryPool.hh:
##########
@@ -42,13 +42,15 @@ namespace orc {
uint64_t currentSize_;
// maximal capacity (actual allocated memory)
uint64_t currentCapacity_;
+ // whether this buffer owns memory lifecycle
+ bool ownBuffer_;
// not implemented
DataBuffer(DataBuffer& buffer);
DataBuffer& operator=(DataBuffer& buffer);
public:
- DataBuffer(MemoryPool& pool, uint64_t size = 0);
+ DataBuffer(MemoryPool& pool, uint64_t size = 0, bool ownBuf = true);
Review Comment:
With the new `ownBuf` constructor parameter, passing `ownBuf=false` while
also passing a non-zero `size` silently results in a buffer with `size()==0`
and `data()==nullptr`. Consider either enforcing `size==0` when `ownBuf==false`
(assert/throw) or documenting the behavior more explicitly to avoid surprising
API usage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
