components/core/src/clp_s/search/clp_search/EncodedVariableInterpreter.cpp (3)

39-39: Consider adding more context or logs upon returning false
When the dictionary returns no entries, returning false implies no match. For traceability and debugging, consider logging a brief message indicating the unmatched variable string.

---

46-51: Avoid shadowing and preserve clarity
The variable encoded_var declared inside this block may overshadow the encoded_var declared above. To improve clarity and maintain scoping hygiene, consider renaming or reusing the existing variable.

---

53-61: Confirm efficiency for large sets of matches
Using std::unordered_set for both the IDs and the entries is a good approach for preventing duplicates. If the dictionary can grow large, ensure that insertion and iteration remain performant.

components/core/src/clp_s/DictionaryReader.hpp (1)

159-181: Evaluate case-sensitive early exit logic
Early exit in the case-sensitive branch returns after the first match. This design is acceptable if the business logic requires only one match; otherwise, you may want to continue searching for potential duplicates. Also, consider that repeatedly converting each dictionary entry’s value to uppercase in the else-branch may affect performance on large dictionaries.

components/core/src/clp/DictionaryReader.hpp (1)

236-258: Ensure consistency in case handling
The code pattern for case-sensitive versus case-insensitive matches is consistent. Similar to the other DictionaryReader, consider potential performance costs of repeatedly calling boost::algorithm::to_upper_copy() on each iteration.

components/core/tests/test-EncodedVariableInterpreter.cpp (2)

379-379: Correct typographical error in test name
The word “metching” should be “matching.”

-    SECTION("Test multiple metching values") {
+    SECTION("Test multiple matching values") {

---

382-405: Comprehensive case-insensitive verification
By storing multiple string variants and confirming that only the case-insensitive retrieval returns all of them, you have validated the updated dictionary logic thoroughly. Consider verifying each returned entry’s content for added rigour.

components/core/src/clp_s/search/Output.cpp (2)

935-935: Consider checking if the returned vector is empty before use.
While it's not strictly necessary, confirming that entries is non-empty (or otherwise handling the empty case) can improve clarity and prevent potential unexpected behaviour in future modifications.

---

940-940: Reserve space in the set before inserting to avoid repeated rehashing.
Since you already know the approximate number of elements from the size of entries, reserving that capacity in matching_vars (e.g., matching_vars.reserve(entries.size())) can marginally improve performance and reduce memory overhead.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

components/core/src/clp_s/DictionaryReader.hpp (2)

69-70: Consider using std::vector<EntryType const*> on a single line.

The return type declaration spans two lines unnecessarily.

-    std::vector<EntryType const*>
-    get_entry_matching_value(std::string const& search_string, bool ignore_case) const;
+    std::vector<EntryType const*> get_entry_matching_value(std::string const& search_string, bool ignore_case) const;

---

159-182: Consider using emplace_back instead of push_back.

The implementation looks good, but consider using emplace_back for potentially better performance when adding entries to the vector.

-                entries.push_back(&entry);
+                entries.emplace_back(&entry);

Also at:

-                entries.push_back(&entry);
+                entries.emplace_back(&entry);

components/core/src/clp/EncodedVariableInterpreter.cpp (3)

389-390: Consider using auto with explicit type for better readability.

The type of entries could be made more explicit while still using auto.

-        auto const entries = var_dict.get_entry_matching_value(var_str, ignore_case);
+        auto const entries = std::vector<VariableDictionaryEntry const*>{var_dict.get_entry_matching_value(var_str, ignore_case)};

---

398-403: Consider using std::size instead of size().

The single entry case is handled correctly, but consider using std::size for consistency with modern C++.

-        if (entries.size() == 1) {
+        if (std::size(entries) == 1) {

---

405-413: Consider reserving space in the sets for better performance.

When dealing with multiple entries, consider reserving space in both sets to avoid reallocations.

         std::unordered_set<encoded_variable_t> encoded_vars;
+        encoded_vars.reserve(entries.size());
         std::unordered_set<clp::VariableDictionaryEntry const*> const entries_set(
                 entries.begin(),
                 entries.end()
         );
         for (auto const& entry : entries) {
             encoded_vars.insert(encode_var_dict_id(entry->get_id()));
         }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

components/core/src/clp/EncodedVariableInterpreter.cpp (1)

404-411: Consider using range-based initialization for the entries set.

The current implementation creates two sets and populates them separately. Consider using range-based initialization for better readability and efficiency.

-        std::unordered_set<encoded_variable_t> encoded_vars;
-        std::unordered_set<clp::VariableDictionaryEntry const*> const entries_set(
-                entries.begin(),
-                entries.end()
-        );
-        for (auto const entry : entries) {
-            encoded_vars.insert(encode_var_dict_id(entry->get_id()));
-        }
+        std::unordered_set<clp::VariableDictionaryEntry const*> const entries_set(entries.begin(), entries.end());
+        std::unordered_set<encoded_variable_t> encoded_vars;
+        encoded_vars.reserve(entries.size());
+        std::transform(entries.begin(), entries.end(), 
+            std::inserter(encoded_vars, encoded_vars.begin()),
+            [this](auto const* entry) { return encode_var_dict_id(entry->get_id()); });

components/core/src/clp_s/search/Output.cpp (1)

940-941: Use const reference in the range-based for loop.

When iterating over entries, use const& to avoid unnecessary copying.

-                for (auto const& entry : entries) {
+                for (auto const* const& entry : entries) {

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

components/core/src/clp_s/DictionaryReader.hpp (1)

177-184: Consider caching the uppercase value of each entry.

For case-insensitive searches, we're converting each entry's value to uppercase on every search. This could be inefficient for large dictionaries with frequent searches.

Consider caching the uppercase value in the EntryType class:

 class DictionaryEntry {
+    private:
+        mutable std::string m_value_uppercase;
+
+    public:
+        std::string const& get_value_uppercase() const {
+            if (m_value_uppercase.empty()) {
+                m_value_uppercase = boost::algorithm::to_upper_copy(m_value);
+            }
+            return m_value_uppercase;
+        }
 };

Then update the search logic:

-        if (boost::algorithm::to_upper_copy(entry.get_value()) == search_string_uppercase) {
+        if (entry.get_value_uppercase() == search_string_uppercase) {

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro