Copilot commented on code in PR #527:
URL: https://github.com/apache/tsfile/pull/527#discussion_r2174932692
##########
cpp/CMakeLists.txt:
##########
@@ -22,70 +22,70 @@ project(TsFile_CPP)
cmake_policy(SET CMP0079 NEW)
set(TsFile_CPP_VERSION 2.1.0.dev)
set(CMAKE_CXX_FLAGS "$ENV{CXXFLAGS} -Wall")
-if(CMAKE_CXX_COMPILER_ID MATCHES "GNU")
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-error=maybe-uninitialized
-D__STDC_FORMAT_MACROS")
-endif()
+if (CMAKE_CXX_COMPILER_ID MATCHES "GNU")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wunused -Wuninitialized -Werror
-Wshadow -D__STDC_FORMAT_MACROS")
Review Comment:
[nitpick] Modifying `CMAKE_CXX_FLAGS` globally can lead to duplicated or
misordered flags; using `target_compile_options` per target may improve
maintainability and avoid unintended side effects.
##########
cpp/src/encoding/bitpack_decoder.h:
##########
@@ -124,17 +124,19 @@ class BitPackDecoder {
delete[] current_buffer_;
}
current_buffer_ = new int64_t[bit_packed_group_count * 8];
- unsigned char bytes[bit_packed_group_count * bit_width_];
int bytes_to_read = bit_packed_group_count * bit_width_;
if (bytes_to_read > (int)byte_cache_.remaining_size()) {
bytes_to_read = byte_cache_.remaining_size();
}
+ std::vector<unsigned char> bytes(bytes_to_read);
+
for (int i = 0; i < bytes_to_read; i++) {
common::SerializationUtil::read_ui8(bytes[i], byte_cache_);
}
+
// save all int values in currentBuffer
packer_->unpack_all_values(
- bytes, bytes_to_read,
+ bytes.data(), bytes_to_read,
Review Comment:
Allocating a std::vector on each decode call can be expensive; consider
reusing a member buffer or using a thread-local/static buffer to avoid repeated
heap allocations in tight loops.
##########
cpp/src/common/allocator/mem_alloc.cc:
##########
@@ -63,45 +63,41 @@ const char *g_mod_names[__LAST_MOD_ID] = {
/* 27 */ "HASH_TABLE",
};
-const uint32_t HEADER_SIZE_4B = 4;
-const uint32_t HEADER_SIZE_8B = 8;
+// Most modern CPUs (e.g., x86_64, Arm) support at least 8-byte alignment,
+// and C++ mandates that alignof(std::max_align_t) reflects the strictest
+// alignment requirement for built-in types (typically 8 or 16 bytes,
especially
+// with SIMD)
+
+// To ensure that the returned memory pointer from mem_alloc is properly
aligned
+// for any type, we standardize on an 8-byte header(HEADER_SIZE_8B).
+// If the actual header content is smaller, additional padding is inserted
+// automatically before the aligned payload to preserve alignment.
+// constexpr uint32_t HEADER_SIZE_4B = 4;
+constexpr size_t HEADER_PTR_SIZE = 8;
+// Default alignment is 8 bytes, sufficient for basic types.
+// If SIMD (e.g., SSE/AVX) is introduced later, increase ALIGNMENT to 16/32/64
+// as needed.
+// constexpr size_t ALIGNMENT = alignof(std::max_align_t);
+constexpr size_t ALIGNMENT = 8;
Review Comment:
Hard-coding `ALIGNMENT` to 8 may not satisfy the strictest alignment
requirements on all platforms; consider using `alignof(std::max_align_t)` or
making this configurable.
```suggestion
constexpr size_t ALIGNMENT = alignof(std::max_align_t);
// constexpr size_t ALIGNMENT = 8;
```
--
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: notifications-unsubscr...@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
