From c5d551da60afd4d8f528712c478c16298a227310 Mon Sep 17 00:00:00 2001 From: Bingran Hu Date: Thu, 21 Nov 2024 23:43:24 -0500 Subject: [PATCH] Address review concern --- components/core/CMakeLists.txt | 1 - components/core/src/clp/clg/CMakeLists.txt | 1 - components/core/src/clp/clo/CMakeLists.txt | 1 - components/core/src/clp/clp/CMakeLists.txt | 1 - .../clp/streaming_compression/Compressor.hpp | 7 +-- .../clp/streaming_compression/Constants.hpp | 13 ------ .../streaming_compression/Decompressor.hpp | 7 +-- .../passthrough/Compressor.cpp | 6 --- .../passthrough/Compressor.hpp | 6 +-- .../passthrough/Decompressor.hpp | 3 +- .../streaming_compression/zstd/Compressor.cpp | 45 +++---------------- .../streaming_compression/zstd/Compressor.hpp | 19 ++++---- .../zstd/Decompressor.cpp | 3 +- .../core/tests/test-StreamingCompression.cpp | 11 +++++ 14 files changed, 34 insertions(+), 90 deletions(-) delete mode 100644 components/core/src/clp/streaming_compression/Constants.hpp diff --git a/components/core/CMakeLists.txt b/components/core/CMakeLists.txt index e5c9b06c8..8d9604abd 100644 --- a/components/core/CMakeLists.txt +++ b/components/core/CMakeLists.txt @@ -460,7 +460,6 @@ set(SOURCE_FILES_unitTest src/clp/streaming_archive/writer/utils.cpp src/clp/streaming_archive/writer/utils.hpp src/clp/streaming_compression/Compressor.hpp - src/clp/streaming_compression/Constants.hpp src/clp/streaming_compression/Decompressor.hpp src/clp/streaming_compression/passthrough/Compressor.cpp src/clp/streaming_compression/passthrough/Compressor.hpp diff --git a/components/core/src/clp/clg/CMakeLists.txt b/components/core/src/clp/clg/CMakeLists.txt index c3c8e3aea..de93cb39c 100644 --- a/components/core/src/clp/clg/CMakeLists.txt +++ b/components/core/src/clp/clg/CMakeLists.txt @@ -89,7 +89,6 @@ set( ../streaming_archive/writer/File.hpp ../streaming_archive/writer/Segment.cpp ../streaming_archive/writer/Segment.hpp - ../streaming_compression/Constants.hpp ../streaming_compression/Decompressor.hpp ../streaming_compression/passthrough/Compressor.cpp ../streaming_compression/passthrough/Compressor.hpp diff --git a/components/core/src/clp/clo/CMakeLists.txt b/components/core/src/clp/clo/CMakeLists.txt index 39cc72b60..6355363bc 100644 --- a/components/core/src/clp/clo/CMakeLists.txt +++ b/components/core/src/clp/clo/CMakeLists.txt @@ -89,7 +89,6 @@ set( ../streaming_archive/writer/File.hpp ../streaming_archive/writer/Segment.cpp ../streaming_archive/writer/Segment.hpp - ../streaming_compression/Constants.hpp ../streaming_compression/Decompressor.hpp ../streaming_compression/passthrough/Compressor.cpp ../streaming_compression/passthrough/Compressor.hpp diff --git a/components/core/src/clp/clp/CMakeLists.txt b/components/core/src/clp/clp/CMakeLists.txt index eff32ce46..0ec3b7147 100644 --- a/components/core/src/clp/clp/CMakeLists.txt +++ b/components/core/src/clp/clp/CMakeLists.txt @@ -117,7 +117,6 @@ set( ../streaming_archive/writer/utils.cpp ../streaming_archive/writer/utils.hpp ../streaming_compression/Compressor.hpp - ../streaming_compression/Constants.hpp ../streaming_compression/Decompressor.hpp ../streaming_compression/passthrough/Compressor.cpp ../streaming_compression/passthrough/Compressor.hpp diff --git a/components/core/src/clp/streaming_compression/Compressor.hpp b/components/core/src/clp/streaming_compression/Compressor.hpp index f1bfecb4e..ac55bd270 100644 --- a/components/core/src/clp/streaming_compression/Compressor.hpp +++ b/components/core/src/clp/streaming_compression/Compressor.hpp @@ -9,7 +9,6 @@ #include "../FileWriter.hpp" #include "../TraceableException.hpp" #include "../WriterInterface.hpp" -#include "Constants.hpp" namespace clp::streaming_compression { /** @@ -31,7 +30,7 @@ class Compressor : public WriterInterface { }; // Constructor - explicit Compressor(CompressorType type) : m_type{type} {} + Compressor() = default; // Destructor virtual ~Compressor() = default; @@ -74,10 +73,6 @@ class Compressor : public WriterInterface { * @param file_writer */ virtual auto open(FileWriter& file_writer) -> void = 0; - -private: - // Variables - CompressorType m_type; }; } // namespace clp::streaming_compression diff --git a/components/core/src/clp/streaming_compression/Constants.hpp b/components/core/src/clp/streaming_compression/Constants.hpp deleted file mode 100644 index 6c6a78bfc..000000000 --- a/components/core/src/clp/streaming_compression/Constants.hpp +++ /dev/null @@ -1,13 +0,0 @@ -#ifndef CLP_STREAMING_COMPRESSION_CONSTANTS_HPP -#define CLP_STREAMING_COMPRESSION_CONSTANTS_HPP - -#include - -namespace clp::streaming_compression { -enum class CompressorType : uint8_t { - ZSTD = 0x10, - Passthrough = 0xFF, -}; -} // namespace clp::streaming_compression - -#endif // CLP_STREAMING_COMPRESSION_CONSTANTS_HPP diff --git a/components/core/src/clp/streaming_compression/Decompressor.hpp b/components/core/src/clp/streaming_compression/Decompressor.hpp index 31666acd9..fa626b2bc 100644 --- a/components/core/src/clp/streaming_compression/Decompressor.hpp +++ b/components/core/src/clp/streaming_compression/Decompressor.hpp @@ -6,7 +6,6 @@ #include "../FileReader.hpp" #include "../ReaderInterface.hpp" #include "../TraceableException.hpp" -#include "Constants.hpp" namespace clp::streaming_compression { class Decompressor : public ReaderInterface { @@ -25,7 +24,7 @@ class Decompressor : public ReaderInterface { }; // Constructor - explicit Decompressor(CompressorType type) : m_compression_type(type) {} + Decompressor() = default; // Destructor ~Decompressor() = default; @@ -57,10 +56,6 @@ class Decompressor : public ReaderInterface { char* extraction_buf, size_t extraction_len ) = 0; - -protected: - // Variables - CompressorType m_compression_type; }; } // namespace clp::streaming_compression diff --git a/components/core/src/clp/streaming_compression/passthrough/Compressor.cpp b/components/core/src/clp/streaming_compression/passthrough/Compressor.cpp index 305c61056..d4ab89dbe 100644 --- a/components/core/src/clp/streaming_compression/passthrough/Compressor.cpp +++ b/components/core/src/clp/streaming_compression/passthrough/Compressor.cpp @@ -4,14 +4,8 @@ #include "../../ErrorCode.hpp" #include "../../TraceableException.hpp" -#include "../Compressor.hpp" -#include "../Constants.hpp" namespace clp::streaming_compression::passthrough { -Compressor::Compressor() - : ::clp::streaming_compression::Compressor{CompressorType::Passthrough}, - m_compressed_stream_file_writer{nullptr} {} - auto Compressor::write(char const* data, size_t const data_length) -> void { if (nullptr == m_compressed_stream_file_writer) { throw OperationFailed(ErrorCode_NotInit, __FILENAME__, __LINE__); diff --git a/components/core/src/clp/streaming_compression/passthrough/Compressor.hpp b/components/core/src/clp/streaming_compression/passthrough/Compressor.hpp index 9ec291c19..f7ccd5004 100644 --- a/components/core/src/clp/streaming_compression/passthrough/Compressor.hpp +++ b/components/core/src/clp/streaming_compression/passthrough/Compressor.hpp @@ -27,8 +27,8 @@ class Compressor : public ::clp::streaming_compression::Compressor { } }; - // Constructors - Compressor(); + // Constructor + Compressor() = default; // Destructor ~Compressor() override = default; @@ -76,7 +76,7 @@ class Compressor : public ::clp::streaming_compression::Compressor { private: // Variables - FileWriter* m_compressed_stream_file_writer; + FileWriter* m_compressed_stream_file_writer{nullptr}; }; } // namespace clp::streaming_compression::passthrough diff --git a/components/core/src/clp/streaming_compression/passthrough/Decompressor.hpp b/components/core/src/clp/streaming_compression/passthrough/Decompressor.hpp index 49501dc6e..8ad87b964 100644 --- a/components/core/src/clp/streaming_compression/passthrough/Decompressor.hpp +++ b/components/core/src/clp/streaming_compression/passthrough/Decompressor.hpp @@ -26,8 +26,7 @@ class Decompressor : public ::clp::streaming_compression::Decompressor { // Constructors Decompressor() - : ::clp::streaming_compression::Decompressor(CompressorType::Passthrough), - m_input_type(InputType::NotInitialized), + : m_input_type(InputType::NotInitialized), m_compressed_data_buf(nullptr), m_compressed_data_buf_len(0), m_decompressed_stream_pos(0) {} diff --git a/components/core/src/clp/streaming_compression/zstd/Compressor.cpp b/components/core/src/clp/streaming_compression/zstd/Compressor.cpp index 0ab9abe9c..74789f4a8 100644 --- a/components/core/src/clp/streaming_compression/zstd/Compressor.cpp +++ b/components/core/src/clp/streaming_compression/zstd/Compressor.cpp @@ -5,47 +5,12 @@ #include #include -#include "../../Array.hpp" #include "../../ErrorCode.hpp" #include "../../FileWriter.hpp" #include "../../TraceableException.hpp" -#include "../Compressor.hpp" -#include "../Constants.hpp" - -namespace { -/** - * Checks if a value returned by ZStd function indicates an error code. - * - * For most ZStd functions that return `size_t` results, instead of returning a union type that can - * either be a valid result or an error code, an unanimous `size_t` type is returned. - * Usually, if the return value exceeds the maximum possible value of valid results, it is treated - * as an error code. However, the exact behavior is function-dependent, so ZStd provides: - * 1. A value checking function `ZSTD_isError` - * 2. A size_t <-> error_code_enum mapping function `ZSTD_getErrorCode`. - * See also: https://facebook.github.io/zstd/zstd_manual.html - * - * @param result A `size_t` type result returned from ZStd APIs - * @return Whether the result is an error code and indicates an error has occurred - */ -auto is_error(size_t result) -> bool { - return 0 != ZSTD_isError(result) && ZSTD_error_no_error != ZSTD_getErrorCode(result); -} -} // namespace namespace clp::streaming_compression::zstd { -Compressor::Compressor() - : ::clp::streaming_compression::Compressor{CompressorType::ZSTD}, - m_compressed_stream_file_writer{nullptr}, - m_compression_stream{ZSTD_createCStream()}, - m_compression_stream_contains_data{false}, - m_compressed_stream_block_size{ZSTD_CStreamOutSize()}, - m_compressed_stream_block_buffer{Array{m_compressed_stream_block_size}}, - m_compressed_stream_block{ - .dst = m_compressed_stream_block_buffer.data(), - .size = m_compressed_stream_block_size, - .pos = 0 - }, - m_uncompressed_stream_pos{0} { +Compressor::Compressor() { if (nullptr == m_compression_stream) { SPDLOG_ERROR("streaming_compression::zstd::Compressor: ZSTD_createCStream() error"); throw OperationFailed(ErrorCode_Failure, __FILENAME__, __LINE__); @@ -63,7 +28,7 @@ auto Compressor::open(FileWriter& file_writer, int compression_level) -> void { // Setup compression stream auto const init_result{ZSTD_initCStream(m_compression_stream, compression_level)}; - if (is_error(init_result)) { + if (ZSTD_error_no_error != ZSTD_getErrorCode(init_result)) { SPDLOG_ERROR( "streaming_compression::zstd::Compressor: ZSTD_initCStream() error: {}", ZSTD_getErrorName(init_result) @@ -106,7 +71,7 @@ auto Compressor::write(char const* data, size_t data_length) -> void { &m_compressed_stream_block, &uncompressed_stream_block )}; - if (is_error(compress_result)) { + if (ZSTD_error_no_error != ZSTD_getErrorCode(compress_result)) { SPDLOG_ERROR( "streaming_compression::zstd::Compressor: ZSTD_compressStream() error: {}", ZSTD_getErrorName(compress_result) @@ -134,7 +99,7 @@ auto Compressor::flush() -> void { m_compressed_stream_block.pos = 0; auto const end_stream_result{ZSTD_endStream(m_compression_stream, &m_compressed_stream_block)}; - if (is_error(end_stream_result)) { + if (ZSTD_error_no_error != ZSTD_getErrorCode(end_stream_result)) { // Note: Output buffer is large enough that it is guaranteed to have enough room to be // able to flush the entire buffer, so this can only be an error SPDLOG_ERROR( @@ -168,7 +133,7 @@ auto Compressor::flush_without_ending_frame() -> void { while (true) { m_compressed_stream_block.pos = 0; auto const flush_result{ZSTD_flushStream(m_compression_stream, &m_compressed_stream_block)}; - if (is_error(flush_result)) { + if (ZSTD_error_no_error != ZSTD_getErrorCode(flush_result)) { SPDLOG_ERROR( "streaming_compression::zstd::Compressor: ZSTD_compressStream2() error: {}", ZSTD_getErrorName(flush_result) diff --git a/components/core/src/clp/streaming_compression/zstd/Compressor.hpp b/components/core/src/clp/streaming_compression/zstd/Compressor.hpp index 0908b4fcc..c2736737e 100644 --- a/components/core/src/clp/streaming_compression/zstd/Compressor.hpp +++ b/components/core/src/clp/streaming_compression/zstd/Compressor.hpp @@ -92,17 +92,20 @@ class Compressor : public ::clp::streaming_compression::Compressor { private: // Variables - FileWriter* m_compressed_stream_file_writer; + FileWriter* m_compressed_stream_file_writer{nullptr}; // Compressed stream variables - ZSTD_CStream* m_compression_stream; - bool m_compression_stream_contains_data; - - size_t m_compressed_stream_block_size; - Array m_compressed_stream_block_buffer; - ZSTD_outBuffer m_compressed_stream_block; + ZSTD_CStream* m_compression_stream{ZSTD_createCStream()}; + bool m_compression_stream_contains_data{false}; + + Array m_compressed_stream_block_buffer{ZSTD_CStreamOutSize()}; + ZSTD_outBuffer m_compressed_stream_block{ + .dst = m_compressed_stream_block_buffer.data(), + .size = m_compressed_stream_block_buffer.size(), + .pos = 0 + }; - size_t m_uncompressed_stream_pos; + size_t m_uncompressed_stream_pos{0}; }; } // namespace clp::streaming_compression::zstd diff --git a/components/core/src/clp/streaming_compression/zstd/Decompressor.cpp b/components/core/src/clp/streaming_compression/zstd/Decompressor.cpp index 818379a24..c3802023c 100644 --- a/components/core/src/clp/streaming_compression/zstd/Decompressor.cpp +++ b/components/core/src/clp/streaming_compression/zstd/Decompressor.cpp @@ -8,8 +8,7 @@ namespace clp::streaming_compression::zstd { Decompressor::Decompressor() - : ::clp::streaming_compression::Decompressor(CompressorType::ZSTD), - m_input_type(InputType::NotInitialized), + : m_input_type(InputType::NotInitialized), m_decompression_stream(nullptr), m_file_reader(nullptr), m_file_reader_initial_pos(0), diff --git a/components/core/tests/test-StreamingCompression.cpp b/components/core/tests/test-StreamingCompression.cpp index 42fbb0721..0fbae9e3a 100644 --- a/components/core/tests/test-StreamingCompression.cpp +++ b/components/core/tests/test-StreamingCompression.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -97,6 +98,16 @@ TEST_CASE("StreamingCompression", "[StreamingCompression]") { num_uncompressed_bytes += chunk_size; } + // Sanity check + REQUIRE( + (std::accumulate( + cCompressionChunkSizes.cbegin(), + cCompressionChunkSizes.cend(), + size_t{0} + ) + == num_uncompressed_bytes) + ); + // Cleanup boost::filesystem::remove(compressed_file_path); }