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 <[email protected]>
y-scope · Apr 25, 2024 · 33647d0 · 33647d0
1 parent 9253255
commit 33647d0
Show file tree

Hide file tree

Showing 11 changed files with 53 additions and 48 deletions.
diff --git 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
@@ -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
@@ -21,18 +21,18 @@ namespace {
      * Generic helper for ir_decoder_decode_*_log_message
      */
     template <class encoded_var_view_t>
-    auto decode_log_message(
+    [[nodiscard]] auto decode_log_message(
             StringView logtype,
             encoded_var_view_t vars,
             StringView dict_vars,
             Int32tSpan dict_var_end_offsets,
             void* ir_decoder,
             StringView* log_msg_view
     ) -> int {
-        typedef typename std::conditional<
+        using encoded_var_t = std::conditional<
                 std::is_same_v<Int64tSpan, encoded_var_view_t>,
                 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<Decoder*>(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
@@ -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
@@ -30,7 +30,7 @@ namespace {
      * Generic helper for ir_deserializer_deserialize_*_log_event
      */
     template <class encoded_variable_t>
-    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 <class encoded_variable_t>
-    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<std::pair<std::string_view, bool>> 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<bool, size_t> 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<int>(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<int>(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<int>(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<int>(IRErrorCode_Success);
         }
     }
 }  // namespace

diff --git 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
@@ -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<Int64tSpan, encoded_var_view_t>,
                 ffi::eight_byte_encoded_variable_t,
-                ffi::four_byte_encoded_variable_t>::type encoded_var_t;
+                ffi::four_byte_encoded_variable_t>::type;
         Encoder<encoded_var_t>* encoder{static_cast<Encoder<encoded_var_t>*>(ir_encoder)};
-        LogMessage<encoded_var_t>& 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
@@ -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
@@ -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
@@ -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
@@ -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