Apply suggestions from code review

Co-authored-by: kirkrodrigues <[email protected]>

components/core/src/clp/ffi/KeyValuePairLogEvent.cpp

@@ -190,14 +190,13 @@ node_type_matches_value_type(SchemaTree::Node::Type type, Value const& value) ->
 [[nodiscard]] auto decode_as_encoded_text_ast(Value const& val) -> std::optional<string>;
 
 /**
- * Serializes the given node ID value pairs into a `nlohmann::json` object.
+ * Serializes the given node-ID-value pairs into a `nlohmann::json` object.
  * @param schema_tree
  * @param node_id_value_pairs
  * @param schema_subtree_bitmap
  * @return A result containing the serialized JSON object or an error code indicating the failure:
- * - std::errc::protocol_error if a value in the log event couldn't be decoded or it couldn't be
+ * - std::errc::protocol_error if a value in the log event couldn't be decoded, or it couldn't be
  *   inserted into a JSON object.
- * - std::errc::result_out_of_range if a node ID in the log event doesn't exist in the schema tree.
  */
 [[nodiscard]] auto serialize_node_id_value_pairs_to_json(
         SchemaTree const& schema_tree,
@@ -210,7 +209,7 @@ node_type_matches_value_type(SchemaTree::Node::Type type, Value const& value) ->
  * @param parent_node_id_to_key_names
  * @return true if `node`'s key is unique among its sibling nodes with `parent_node_id_to_key_names`
  * updated to keep track of this unique key name.
- * @return false if `node`'s key already exists in other sibling nodes.
+ * @return false if a sibling of `node` has the same key.
  */
 [[nodiscard]] auto check_key_uniqueness_among_sibling_nodes(
         SchemaTree::Node const& node,
@@ -276,12 +275,12 @@ auto validate_node_id_value_pairs(
                 return std::errc::protocol_not_supported;
             }
 
-            // Iteratively check if there's any key duplication in ancestors until:
-            // 1. The ancestor is already checked. We only need to check an ancestor node once. If
-            //    there are key duplications among its sibling, it will be caught when the sibling
-            //    is first checked. The order of which sibling gets checked first doesn't affect the
-            //    results.
-            // 2. Reached the root node.
+            // Iteratively check if there's any key duplication in the node's ancestors until:
+            // 1. The ancestor has already been checked. We only need to check an ancestor node
+            //    once since if there are key duplications among its siblings, it would've been
+            //    caught when the sibling was first checked (the order in which siblings get checked
+            //    doesn't affect the results).
+            // 2. We reach the root node.
             auto next_ancestor_node_id_to_check{node.get_parent_id_unsafe()};
             while (false == key_duplication_checked_node_id_bitmap[next_ancestor_node_id_to_check])
             {
@@ -335,7 +334,7 @@ auto get_schema_subtree_bitmap(
         KeyValuePairLogEvent::NodeIdValuePairs const& node_id_value_pairs,
         SchemaTree const& schema_tree
 ) -> OUTCOME_V2_NAMESPACE::std_result<vector<bool>> {
-    auto schema_subtree_bitmap{vector<bool>(schema_tree.get_size(), false)};
+    vector<bool> schema_subtree_bitmap(schema_tree.get_size(), false);
     for (auto const& [node_id, val] : node_id_value_pairs) {
         if (node_id >= schema_subtree_bitmap.size()) {
             return std::errc::result_out_of_range;
@@ -505,8 +504,8 @@ auto check_key_uniqueness_among_sibling_nodes(
         std::unordered_map<SchemaTree::Node::id_t, std::unordered_set<std::string_view>>&
                 parent_node_id_to_key_names
 ) -> bool {
-    // The given node must not be the root (checked by the caller), so we can query the underlying
-    // ID safely without a repeated check.
+    // The caller checks that the given node is not the root, so we can query the underlying
+    // parent ID safely without a check.
     auto const parent_node_id{node.get_parent_id_unsafe()};
     auto const key_name{node.get_key_name()};
     auto const parent_node_id_to_key_names_it{parent_node_id_to_key_names.find(parent_node_id)};

components/core/src/clp/ffi/KeyValuePairLogEvent.hpp

@@ -17,12 +17,12 @@
 namespace clp::ffi {
 /**
  * A log event containing key-value pairs. Each event contains:
- * - A reference to the schema tree of auto-generated keys.
- * - A reference to the schema tree of user-generated keys.
+ * - A reference to the schema tree for auto-generated keys.
+ * - A reference to the schema tree for user-generated keys.
  * - A collection of auto-generated node-ID & value pairs, where each pair represents a leaf
- *   `SchemaTree::Node` in the `SchemaTree`.
+ *   `SchemaTree::Node` in the schema tree for auto-generated keys.
  * - A collection of user-generated node-ID & value pairs, where each pair represents a leaf
- *   `SchemaTree::Node` in the `SchemaTree`.
+ *   `SchemaTree::Node` in the schema tree for user-generated keys.
  * - The UTC offset of the current log event.
  */
 class KeyValuePairLogEvent {
@@ -38,7 +38,7 @@ class KeyValuePairLogEvent {
      * @param user_generated_node_id_value_pairs
      * @param utc_offset
      * @return A result containing the key-value pair log event or an error code indicating the
-     * failure, or an error code indicating the failure:
+     * failure:
      * - std::errc::invalid_argument if any of the given schema tree pointers are null.
      * - Forwards `validate_node_id_value_pairs`'s return values.
      */
@@ -80,9 +80,9 @@ class KeyValuePairLogEvent {
 
     /**
      * @return A result containing a bitmap where every bit corresponds to the ID of a node in the
-     * auto-generated schema tree, and the set bits correspond to the nodes in the subtree defined
-     * by all paths from the root node to the nodes in `m_auto_generated_node_id_value_pairs`; or an
-     * error code indicating a failure:
+     * schema tree for auto-generated keys, and the set bits correspond to the nodes in the subtree
+     * defined by all paths from the root node to the nodes in
+     * `m_auto_generated_node_id_value_pairs`; or an error code indicating a failure:
      * - std::errc::result_out_of_range if a node ID in `m_auto_generated_node_id_value_pairs`
      *   doesn't exist in the schema tree.
      */
@@ -91,9 +91,9 @@ class KeyValuePairLogEvent {
 
     /**
      * @return A result containing a bitmap where every bit corresponds to the ID of a node in the
-     * user-generated schema tree, and the set bits correspond to the nodes in the subtree defined
-     * by all paths from the root node to the nodes in `m_user_generated_node_id_value_pairs`; or an
-     * error code indicating a failure:
+     * schema tree for user-generated keys, and the set bits correspond to the nodes in the subtree
+     * defined by all paths from the root node to the nodes in
+     * `m_user_generated_node_id_value_pairs`; or an error code indicating a failure:
      * - std::errc::result_out_of_range if a node ID in `m_user_generated_node_id_value_pairs`
      *   doesn't exist in the schema tree.
      */
@@ -109,6 +109,7 @@ class KeyValuePairLogEvent {
      *   - Serialized auto-generated key-value pairs as a JSON object
      *   - Serialized user-generated key-value pairs as a JSON object
      * - The possible error codes:
+     *   - Forwards `get_auto_generated_schema_subtree_bitmap`'s return values on failure.
      *   - Forwards `serialize_node_id_value_pairs_to_json`'s return values on failure.
      */
     [[nodiscard]] auto serialize_to_json(

components/core/src/clp/ffi/SchemaTree.hpp

@@ -127,8 +127,7 @@ class SchemaTree {
         // Destructor
         ~Node() = default;
 
-        // Defines default equal-to operator
-        [[nodiscard]] auto operator==(SchemaTree::Node const& rhs) const -> bool = default;
+        [[nodiscard]] auto operator==(Node const& rhs) const -> bool = default;
 
         // Methods
         [[nodiscard]] auto get_id() const -> id_t { return m_id; }
@@ -251,7 +250,6 @@ class SchemaTree {
     // Destructor
     ~SchemaTree() = default;
 
-    // Equal-to operator
     [[nodiscard]] auto operator==(SchemaTree const& rhs) const -> bool {
         return m_tree_nodes == rhs.m_tree_nodes;
     }

components/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.hpp

@@ -57,9 +57,9 @@ namespace clp::ffi::ir_stream {
  * Deserializes a key-value pair log event IR unit.
  * @param reader
  * @param tag
- * @param auto_generated_schema_tree Schema tree of auto-generated keys used to construct the
+ * @param auto_generated_schema_tree Schema tree for auto-generated keys, used to construct the
  * KV-pair log event.
- * @param user_generated_schema_tree Schema tree of user-generated keys used to construct the
+ * @param user_generated_schema_tree Schema tree for user-generated keys, used to construct the
  * KV-pair log event.
  * @param utc_offset UTC offset used to construct the KV-pair log event.
  * @return A result containing the deserialized log event or an error code indicating the

components/core/tests/test-ffi_KeyValuePairLogEvent.cpp

@@ -83,7 +83,7 @@ auto insert_invalid_node_id_value_pairs_with_node_type_errors(
 ) -> void;
 
 /**
- * Asserts the kv pair log event creation fails with the expected error code.
+ * Asserts that `KeyValuePairLogEvent` creation fails with the expected error code.
  * @param auto_generated_schema_tree
  * @param user_generated_schema_tree
  * @param auto_generated_node_id_value_pairs
@@ -496,7 +496,7 @@ TEST_CASE("ffi_KeyValuePairLogEvent_create", "[ffi]") {
             REQUIRE((serialized_user_generated_kv_pairs == expected));
         }
 
-        SECTION("Test duplicated key conflict on node #3") {
+        SECTION("Test duplicated key conflict under node #3") {
             auto invalid_node_id_value_pairs{valid_node_id_value_pairs};
             // NOLINTNEXTLINE(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
             invalid_node_id_value_pairs.emplace(6, Value{static_cast<value_bool_t>(false)});
@@ -518,7 +518,7 @@ TEST_CASE("ffi_KeyValuePairLogEvent_create", "[ffi]") {
             ));
         }
 
-        SECTION("Test duplicated key conflict on node #4") {
+        SECTION("Test duplicated key conflict under node #4") {
             auto invalid_node_id_value_pairs{valid_node_id_value_pairs};
             // NOLINTNEXTLINE(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
             invalid_node_id_value_pairs.emplace(9, Value{static_cast<value_float_t>(0.0)});
@@ -540,7 +540,7 @@ TEST_CASE("ffi_KeyValuePairLogEvent_create", "[ffi]") {
             ));
         }
 
-        SECTION("Test duplicated keys among siblings of node #1") {
+        SECTION("Test duplicated keys amongst siblings of node #1") {
             auto invalid_node_id_value_pairs{valid_node_id_value_pairs};
             // NOLINTNEXTLINE(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
             invalid_node_id_value_pairs.emplace(12, static_cast<value_int_t>(0));
@@ -563,11 +563,11 @@ TEST_CASE("ffi_KeyValuePairLogEvent_create", "[ffi]") {
             ));
         }
 
-        SECTION("Test duplicated keys among siblings of node #3") {
+        SECTION("Test duplicated keys amongst siblings of node #3") {
             auto invalid_node_id_value_pairs{valid_node_id_value_pairs};
             // NOLINTNEXTLINE(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
             invalid_node_id_value_pairs.emplace(13, false);
-            // Node #12 has the same key as its sibling node #3
+            // Node #13 has the same key as its sibling node #3
             REQUIRE(assert_kv_pair_log_event_creation_failure(
                     auto_generated_schema_tree,
                     user_generated_schema_tree,