From 33647d0485119820e8382f7367dfdc0069e3b7ac Mon Sep 17 00:00:00 2001 From: davidlion Date: Thu, 25 Apr 2024 12:53:00 -0400 Subject: [PATCH] Apply suggestions from code review - fix typos - update NamespaceIndentation to None - add [[nodiscard]] - typedef -> using - add const - use fancy auto syntax Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com> --- README.rst | 2 +- cpp/.clang-format | 2 +- cpp/src/ffi_go/ir/decoder.cpp | 8 +++--- cpp/src/ffi_go/ir/decoder.h | 2 +- cpp/src/ffi_go/ir/deserializer.cpp | 36 +++++++++++++----------- cpp/src/ffi_go/ir/deserializer.h | 2 +- cpp/src/ffi_go/ir/encoder.cpp | 18 ++++++------ cpp/src/ffi_go/ir/encoder.h | 4 +-- cpp/src/ffi_go/ir/serializer.h | 12 ++++---- cpp/src/ffi_go/search/wildcard_query.cpp | 5 ++-- ffi/ffi.go | 10 +++---- 11 files changed, 53 insertions(+), 48 deletions(-) diff --git a/README.rst b/README.rst index a12cc12..1334ac8 100644 --- a/README.rst +++ b/README.rst @@ -78,7 +78,7 @@ Testing ------- To run all unit tests run: ``go_test_ir="/path/to/my-ir.clp.zst" go test ./...`` -- Some of the ``ir`` package's tests currently requries an existing CLP IR file +- Some of the ``ir`` package's tests currently require an existing CLP IR file compressed with zstd. This file's path is taken as an environment variable named ``go_test_ir``. It can be an absolute path or a path relative to the ``ir`` directory. diff --git a/cpp/.clang-format b/cpp/.clang-format index 08627a5..1e8df9a 100644 --- a/cpp/.clang-format +++ b/cpp/.clang-format @@ -100,7 +100,7 @@ KeepEmptyLinesAtTheStartOfBlocks: false LambdaBodyIndentation: Signature LineEnding: LF MaxEmptyLinesToKeep: 1 -NamespaceIndentation: Inner +NamespaceIndentation: None PPIndentWidth: -1 PackConstructorInitializers: CurrentLine PenaltyBreakAssignment: 50 diff --git a/cpp/src/ffi_go/ir/decoder.cpp b/cpp/src/ffi_go/ir/decoder.cpp index f5fd268..49c7c14 100644 --- a/cpp/src/ffi_go/ir/decoder.cpp +++ b/cpp/src/ffi_go/ir/decoder.cpp @@ -21,7 +21,7 @@ namespace { * Generic helper for ir_decoder_decode_*_log_message */ template - auto decode_log_message( + [[nodiscard]] auto decode_log_message( StringView logtype, encoded_var_view_t vars, StringView dict_vars, @@ -29,10 +29,10 @@ namespace { void* ir_decoder, StringView* log_msg_view ) -> int { - typedef typename std::conditional< + using encoded_var_t = std::conditional< std::is_same_v, ffi::eight_byte_encoded_variable_t, - ffi::four_byte_encoded_variable_t>::type encoded_var_t; + ffi::four_byte_encoded_variable_t>::type; Decoder* decoder{static_cast(ir_decoder)}; ffi_go::LogMessage& log_msg = decoder->m_log_message; log_msg.reserve(logtype.m_size + dict_vars.m_size); @@ -47,7 +47,7 @@ namespace { dict_var_end_offsets.m_data, dict_var_end_offsets.m_size ); - } catch (ffi::EncodingException& e) { + } catch (ffi::EncodingException const& e) { err = IRErrorCode_Decode_Error; } diff --git a/cpp/src/ffi_go/ir/decoder.h b/cpp/src/ffi_go/ir/decoder.h index 3c59ea1..52a58d3 100644 --- a/cpp/src/ffi_go/ir/decoder.h +++ b/cpp/src/ffi_go/ir/decoder.h @@ -38,7 +38,7 @@ extern "C" { * @param[in] vars Array of encoded variables * @param[in] dict_vars String containing all dictionary variables concatenated * together - * @param[in] dict_var_end_offsets Array of offsets into dict_vars makring the + * @param[in] dict_var_end_offsets Array of offsets into dict_vars marking the * end of a dictionary variable * @param[in] ir_decoder ir::Decoder to be used as storage for the decoded log * message diff --git a/cpp/src/ffi_go/ir/deserializer.cpp b/cpp/src/ffi_go/ir/deserializer.cpp index b5da8b8..d9e4610 100644 --- a/cpp/src/ffi_go/ir/deserializer.cpp +++ b/cpp/src/ffi_go/ir/deserializer.cpp @@ -30,7 +30,7 @@ namespace { * Generic helper for ir_deserializer_deserialize_*_log_event */ template - auto deserialize_log_event( + [[nodiscard]] auto deserialize_log_event( ByteSpan ir_view, void* ir_deserializer, size_t* ir_pos, @@ -41,7 +41,7 @@ namespace { * Generic helper for ir_deserializer_deserialize_*_wildcard_match */ template - auto deserialize_wildcard_match( + [[nodiscard]] auto deserialize_wildcard_match( ByteSpan ir_view, void* ir_deserializer, TimestampInterval time_interval, @@ -121,8 +121,9 @@ namespace { std::vector> queries(merged_query.m_end_offsets.m_size); size_t pos{0}; for (size_t i{0}; i < merged_query.m_end_offsets.m_size; i++) { - queries[i].first = query_view.substr(pos, end_offsets[i]); - queries[i].second = case_sensitivity[i]; + auto& [query_str_view, is_case_sensitive]{queries[i]}; + query_str_view = query_view.substr(pos, end_offsets[i]); + is_case_sensitive = case_sensitivity[i]; pos += end_offsets[i]; } @@ -178,19 +179,22 @@ namespace { if (time_interval.m_lower > deserializer->m_timestamp) { continue; } - std::pair const match{query_fn(deserializer->m_log_event.m_log_message)}; - if (match.first) { - size_t pos{0}; - if (ErrorCode_Success != ir_buf.try_get_pos(pos)) { - return static_cast(IRErrorCode_Decode_Error); - } - *ir_pos = pos; - log_event->m_log_message.m_data = deserializer->m_log_event.m_log_message.data(); - log_event->m_log_message.m_size = deserializer->m_log_event.m_log_message.size(); - log_event->m_timestamp = deserializer->m_timestamp; - *matching_query = match.second; - return static_cast(IRErrorCode_Success); + auto const [has_matching_query, matching_query_idx]{ + query_fn(deserializer->m_log_event.m_log_message) + }; + if (false == has_matching_query) { + continue; + } + size_t pos{0}; + if (ErrorCode_Success != ir_buf.try_get_pos(pos)) { + return static_cast(IRErrorCode_Decode_Error); } + *ir_pos = pos; + log_event->m_log_message.m_data = deserializer->m_log_event.m_log_message.data(); + log_event->m_log_message.m_size = deserializer->m_log_event.m_log_message.size(); + log_event->m_timestamp = deserializer->m_timestamp; + *matching_query = matching_query_idx; + return static_cast(IRErrorCode_Success); } } } // namespace diff --git a/cpp/src/ffi_go/ir/deserializer.h b/cpp/src/ffi_go/ir/deserializer.h index 7ad65f7..967d4da 100644 --- a/cpp/src/ffi_go/ir/deserializer.h +++ b/cpp/src/ffi_go/ir/deserializer.h @@ -22,7 +22,7 @@ extern "C" { void ir_deserializer_close(void* ir_deserializer); /** - * Given a CLP IR buffer (any encoding), attempt to deserialize a premable and + * Given a CLP IR buffer (any encoding), attempt to deserialize a preamble and * extract its information. An ir::Deserializer will be allocated to use as the * backing storage for a Go ir.Deserializer (i.e. subsequent calls to * ir_deserializer_deserialize_*_log_event). It is left to the Go layer to read diff --git a/cpp/src/ffi_go/ir/encoder.cpp b/cpp/src/ffi_go/ir/encoder.cpp index 54ae633..6be86ae 100644 --- a/cpp/src/ffi_go/ir/encoder.cpp +++ b/cpp/src/ffi_go/ir/encoder.cpp @@ -30,12 +30,12 @@ namespace { StringView* dict_vars, Int32tSpan* dict_var_end_offsets ) -> int { - typedef typename std::conditional< + using encoded_var_t std::conditional< std::is_same_v, ffi::eight_byte_encoded_variable_t, - ffi::four_byte_encoded_variable_t>::type encoded_var_t; + ffi::four_byte_encoded_variable_t>::type; Encoder* encoder{static_cast*>(ir_encoder)}; - LogMessage& ir_log_msg = encoder->m_log_message; + auto& ir_log_msg{encoder->m_log_message}; ir_log_msg.reserve(log_message.m_size); std::string_view const log_msg_view{log_message.m_data, log_message.m_size}; @@ -53,14 +53,14 @@ namespace { // dict_var_offsets contains begin_pos followed by end_pos of each // dictionary variable in the message - int32_t prev_end_off = 0; + int32_t prev_end_off{0}; for (size_t i = 0; i < dict_var_offsets.size(); i += 2) { - int32_t const begin_pos = dict_var_offsets[i]; - int32_t const end_pos = dict_var_offsets[i + 1]; + int32_t const begin_pos{dict_var_offsets[i]}; + int32_t const end_pos{dict_var_offsets[i + 1]}; ir_log_msg.m_dict_vars.insert( - ir_log_msg.m_dict_vars.begin() + prev_end_off, - log_msg_view.begin() + begin_pos, - log_msg_view.begin() + end_pos + ir_log_msg.m_dict_vars.cbegin() + prev_end_off, + log_msg_view.cbegin() + begin_pos, + log_msg_view.cbegin() + end_pos ); prev_end_off = prev_end_off + (end_pos - begin_pos); ir_log_msg.m_dict_var_end_offsets.push_back(prev_end_off); diff --git a/cpp/src/ffi_go/ir/encoder.h b/cpp/src/ffi_go/ir/encoder.h index b8d78e3..c011c11 100644 --- a/cpp/src/ffi_go/ir/encoder.h +++ b/cpp/src/ffi_go/ir/encoder.h @@ -52,7 +52,7 @@ extern "C" { * @param[out] vars Array of encoded variables * @param[out] dict_vars String containing all dictionary variables concatenated * together - * @param[out] dict_var_end_offsets Array of offsets into dict_vars makring the + * @param[out] dict_var_end_offsets Array of offsets into dict_vars marking the * end of a dictionary variable * @return ffi::ir_stream::IRErrorCode_Corrupted_IR if ffi::encode_message * returns false @@ -80,7 +80,7 @@ extern "C" { * @param[out] vars Array of encoded variables * @param[out] dict_vars String containing all dictionary variables concatenated * together - * @param[out] dict_var_end_offsets Array of offsets into dict_vars makring the + * @param[out] dict_var_end_offsets Array of offsets into dict_vars marking the * end of a dictionary variable * @return ffi::ir_stream::IRErrorCode_Corrupted_IR if ffi::encode_message * returns false diff --git a/cpp/src/ffi_go/ir/serializer.h b/cpp/src/ffi_go/ir/serializer.h index 0e200a8..576cbe1 100644 --- a/cpp/src/ffi_go/ir/serializer.h +++ b/cpp/src/ffi_go/ir/serializer.h @@ -17,7 +17,7 @@ void ir_serializer_close(void* ir_serializer); /** - * Given the fields of a CLP IR premable, serialize them into an IR byte stream + * Given the fields of a CLP IR preamble, serialize them into an IR byte stream * with eight byte encoding. An ir::Serializer will be allocated to use as the * backing storage for a Go ir.Serializer (i.e. subsequent calls to * ir_serializer_serialize_*_log_event). All pointer parameters must be non-null @@ -29,7 +29,7 @@ void ir_serializer_close(void* ir_serializer); * @param[in] time_zone_id TZID timezone of the timestamps in the IR * @param[out] ir_serializer_ptr Address of a new ir::Serializer * @param[out] ir_view View of a IR buffer containing the serialized preamble - * @return ffi::ir_stream::IRErrorCode forwared from + * @return ffi::ir_stream::IRErrorCode forwarded from * ffi::ir_stream::eight_byte_encoding::encode_preamble */ int ir_serializer_serialize_eight_byte_preamble( @@ -41,7 +41,7 @@ int ir_serializer_serialize_eight_byte_preamble( ); /** - * Given the fields of a CLP IR premable, serialize them into an IR byte stream + * Given the fields of a CLP IR preamble, serialize them into an IR byte stream * with four byte encoding. An ir::Serializer will be allocated to use as the * backing storage for a Go ir.Serializer (i.e. subsequent calls to * ir_serializer_serialize_*_log_event). All pointer parameters must be non-null @@ -53,7 +53,7 @@ int ir_serializer_serialize_eight_byte_preamble( * @param[in] time_zone_id TZID timezone of the timestamps in the IR * @param[out] ir_serializer_ptr Address of a new ir::Serializer * @param[out] ir_view View of a IR buffer containing the serialized preamble - * @return ffi::ir_stream::IRErrorCode forwared from + * @return ffi::ir_stream::IRErrorCode forwarded from * ffi::ir_stream::four_byte_encoding::encode_preamble */ int ir_serializer_serialize_four_byte_preamble( @@ -74,7 +74,7 @@ int ir_serializer_serialize_four_byte_preamble( * @param[in] timestamp Timestamp of the log event to serialize * @param[in] ir_serializer ir::Serializer object to be used as storage * @param[out] ir_view View of a IR buffer containing the serialized log event - * @return ffi::ir_stream::IRErrorCode forwared from + * @return ffi::ir_stream::IRErrorCode forwarded from * ffi::ir_stream::eight_byte_encoding::encode_message */ int ir_serializer_serialize_eight_byte_log_event( @@ -94,7 +94,7 @@ int ir_serializer_serialize_eight_byte_log_event( * IR stream * @param[in] ir_serializer ir::Serializer object to be used as storage * @param[out] ir_view View of a IR buffer containing the serialized log event - * @return ffi::ir_stream::IRErrorCode forwared from + * @return ffi::ir_stream::IRErrorCode forwarded from * ffi::ir_stream::four_byte_encoding::encode_message */ int ir_serializer_serialize_four_byte_log_event( diff --git a/cpp/src/ffi_go/search/wildcard_query.cpp b/cpp/src/ffi_go/search/wildcard_query.cpp index f3f4380..9ea2360 100644 --- a/cpp/src/ffi_go/search/wildcard_query.cpp +++ b/cpp/src/ffi_go/search/wildcard_query.cpp @@ -14,8 +14,9 @@ extern "C" auto wildcard_query_delete(void* str) -> void { } extern "C" auto wildcard_query_clean(StringView query, void** ptr) -> StringView { - auto* clean = new std::string{ - clean_up_wildcard_search_string(std::string_view{query.m_data, query.m_size})}; + auto* clean{new std::string{ + clean_up_wildcard_search_string(std::string_view{query.m_data, query.m_size}) + }}; *ptr = clean; return {clean->data(), clean->size()}; } diff --git a/ffi/ffi.go b/ffi/ffi.go index 1d6c738..b0d3ef7 100644 --- a/ffi/ffi.go +++ b/ffi/ffi.go @@ -10,7 +10,7 @@ type EpochTimeMs int64 // A LogMessageView is a LogMessage that is backed by C++ allocated memory // rather than the Go heap. A LogMessageView, x, is valid when returned and will // remain valid until a new LogMessageView is returned by the same object (e.g. -// an ir.Deserializer) that retuend x. +// an ir.Deserializer) that returns x. type ( LogMessageView = string LogMessage = string @@ -23,10 +23,10 @@ type LogEvent struct { Timestamp EpochTimeMs } -// The underlying memory of LogEventView is C-allocated and owned by the object -// (e.g. reader, desializer, etc) that returned it. Using an existing -// LogEventView after a new view has been returned by the same producing object -// is undefined (different producing objects own their own memory for views). +// LogEventView memory is allocated and owned by the C++ object (e.g., reader, +// deserializer) that returns it. Reusing a LogEventView after the same object +// has issued a new view leads to undefined behavior, as different objects +// manage their own memory independently. type LogEventView struct { LogMessageView Timestamp EpochTimeMs