Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(core): Fix clang-tidy warnings in the streaming compressor interface and its implementations; Modernize and refactor test-StreamingCompression for conciseness. #599

Merged
merged 12 commits into from
Nov 23, 2024
1 change: 0 additions & 1 deletion components/core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion components/core/src/clp/clg/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion components/core/src/clp/clo/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion components/core/src/clp/clp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 2 additions & 7 deletions components/core/src/clp/streaming_compression/Compressor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@
#include "../FileWriter.hpp"
#include "../TraceableException.hpp"
#include "../WriterInterface.hpp"
#include "Constants.hpp"

namespace clp::streaming_compression {
/**
* Generic compressor interface.
* Abstract compressor interface.
*/
class Compressor : public WriterInterface {
public:
Expand All @@ -31,7 +30,7 @@ class Compressor : public WriterInterface {
};

// Constructor
explicit Compressor(CompressorType type) : m_type{type} {}
Compressor() = default;

// Destructor
virtual ~Compressor() = default;
Expand Down Expand Up @@ -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

Expand Down
13 changes: 0 additions & 13 deletions components/core/src/clp/streaming_compression/Constants.hpp

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include "../FileReader.hpp"
#include "../ReaderInterface.hpp"
#include "../TraceableException.hpp"
#include "Constants.hpp"

namespace clp::streaming_compression {
class Decompressor : public ReaderInterface {
Expand All @@ -25,7 +24,7 @@ class Decompressor : public ReaderInterface {
};

// Constructor
explicit Decompressor(CompressorType type) : m_compression_type(type) {}
Decompressor() = default;

// Destructor
~Decompressor() = default;
Expand Down Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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__);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ class Compressor : public ::clp::streaming_compression::Compressor {
}
};

// Constructors
Compressor();
// Constructor
Compressor() = default;

// Destructor
~Compressor() override = default;
Expand Down Expand Up @@ -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

Expand Down
Bill-hbrhbr marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
Expand Down
47 changes: 6 additions & 41 deletions components/core/src/clp/streaming_compression/zstd/Compressor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,47 +5,12 @@
#include <spdlog/spdlog.h>
#include <zstd_errors.h>

#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 by ZStd functions
* @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<char>{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__);
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -168,9 +133,9 @@ 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: {}",
"streaming_compression::zstd::Compressor: ZSTD_flushStream() error: {}",
ZSTD_getErrorName(flush_result)
);
throw OperationFailed(ErrorCode_Failure, __FILENAME__, __LINE__);
Expand Down
19 changes: 11 additions & 8 deletions components/core/src/clp/streaming_compression/zstd/Compressor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<char> 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<char> m_compressed_stream_block_buffer{ZSTD_CStreamOutSize()};
ZSTD_outBuffer m_compressed_stream_block{
Bill-hbrhbr marked this conversation as resolved.
Show resolved Hide resolved
.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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
13 changes: 12 additions & 1 deletion components/core/tests/test-StreamingCompression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include <array>
#include <cstring>
#include <memory>
#include <numeric>
#include <string>

#include <boost/filesystem/operations.hpp>
Expand Down Expand Up @@ -37,7 +38,7 @@ TEST_CASE("StreamingCompression", "[StreamingCompression]") {
cBufferSize / 2,
cBufferSize}
);
constexpr size_t cAlphabetLength = 26;
constexpr size_t cAlphabetLength{26};
std::string const compressed_file_path{"test_streaming_compressed_file.bin"};

// Initialize compression devices
Expand Down Expand Up @@ -97,6 +98,16 @@ TEST_CASE("StreamingCompression", "[StreamingCompression]") {
num_uncompressed_bytes += chunk_size;
}
Bill-hbrhbr marked this conversation as resolved.
Show resolved Hide resolved

// Sanity check
REQUIRE(
(std::accumulate(
cCompressionChunkSizes.cbegin(),
cCompressionChunkSizes.cend(),
size_t{0}
)
== num_uncompressed_bytes)
);

// Cleanup
boost::filesystem::remove(compressed_file_path);
}
Loading