Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ set(SOURCE_FILES_unitTest
src/clp/Defs.h
src/clp/dictionary_utils.cpp
src/clp/dictionary_utils.hpp
src/clp/DictionaryConcepts.hpp
src/clp/DictionaryEntry.hpp
src/clp/DictionaryReader.hpp
src/clp/DictionaryWriter.hpp
Expand Down
262 changes: 262 additions & 0 deletions components/core/src/clp/DictionaryConcepts.hpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized for dictionary readers, the signatures are almost the same for logtypes and variables. Do u think it's worth making a general dictionary reader requirement to include the common requirements?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit on the fence about this one -- it will probably save a few lines of code but I think it might end up harder to read.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kk sure~

Original file line number Diff line number Diff line change
@@ -0,0 +1,262 @@
#ifndef CLP_DICTIONARYCONCEPTS_HPP
#define CLP_DICTIONARYCONCEPTS_HPP

#include <concepts>
#include <cstddef>
#include <string>
#include <string_view>
#include <unordered_set>
#include <vector>

#include "Defs.h"
#include "ir/types.hpp"

namespace clp {
/**
* Requirement for the logtype dictionary entry interface.
* @tparam LogTypeDictionaryEntryType The type of the logtype dictionary entry.
*/
template <typename LogTypeDictionaryEntryType>
concept LogTypeDictionaryEntryReq = requires(
LogTypeDictionaryEntryType entry,
size_t length,
std::string_view msg,
size_t& begin_pos_ref,
size_t& end_pos_ref,
std::string_view& parsed_var_ref,
size_t begin_pos,
size_t placeholder_ix,
ir::VariablePlaceholder& placeholder_ref,
std::string& logtype
) {
/**
* Clears all internal state.
*/
{
entry.clear()
} -> std::same_as<void>;

/**
* Reserves space for a logtype string of a given length.
* @param length
*/
{
entry.reserve_constant_length(length)
} -> std::same_as<void>;

/**
* Parses the next variable from a message according to `ir::get_bounds_of_next_var`,
* constructing the constant part of the message's logtype in the processed range at the same
* time.
* @param msg The original log message.
* @param begin_pos_ref The beginning position of the last variable. This parameter is updated
* to the beginning position of the next variable.
* @param end_pos_ref The ending position of the last variable (exclusive). This parameter is
* updated to the ending position of the next variable (exclusive).
* @return Whether another variable was found or not.
*/
{
entry.parse_next_var(msg, begin_pos_ref, end_pos_ref, parsed_var_ref)
} -> std::same_as<bool>;

/**
* Adds a substring of `msg` to the constant part of the logtype.
* @param msg The value containing a constant to add to the logtype.
* @param begin_pos The starting offset into `msg` to add to the logtype.
* @param length The length of the substring to add to the logtype.
*/
{
entry.add_constant(msg, begin_pos, length)
} -> std::same_as<void>;

/**
* Adds an integer variable placeholder to the constant part of the logtype.
*/
{
entry.add_int_var()
} -> std::same_as<void>;

/**
* Adds a float variable placeholder to the constant part of the logtype.
*/
{
entry.add_float_var()
} -> std::same_as<void>;

/**
* Adds a dictionary variable placeholder to the constant part of the logtype.
*/
{
entry.add_dictionary_var()
} -> std::same_as<void>;

/**
* @return The constant part of the logtype.
*/
{
entry.get_value()
} -> std::same_as<std::string const&>;

/**
* @return The number of variables in the constant part of the logtype.
*/
{
entry.get_num_variables()
} -> std::same_as<size_t>;

/**
* @return The number of variable placeholders (including escaped ones) in the constant part of
* the logtype.
*/
{
entry.get_num_placeholders()
} -> std::same_as<size_t>;

/**
* Gets the position and type of a variable placeholder in the logtype.
* @param placeholder_ix The index of the placeholder to get the info for.
* @param placeholder_ref The type of the placeholder at `placeholder_ix`.
* @return The placeholder's position in the logtype, or `SIZE_MAX` if `placeholder_ix` is out
* of bounds.
*/
{
entry.get_placeholder_info(placeholder_ix, placeholder_ref)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placeholder_ix -> placeholder_idx
We will stick to idx and drop the use ix for any new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I started using ix over idx because @kirkrodrigues recently told me that ix is preferred. Are we changing that again?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, that might've been a miscommunication. ix is what we have in the codebase cause I used to use that for both iterators and indices, but idx is more conventional.

} -> std::same_as<size_t>;

/**
* @return The dictionary Id for this logtype.
*/
{
entry.get_id()
} -> std::same_as<logtype_dictionary_id_t>;
};

/**
* Requirement for the variable dictionary entry interface.
* @tparam VariableDictionaryEntryType The type of the variable dictionary entry.
*/
template <typename VariableDictionaryEntryType>
concept VariableDictionaryEntryReq = requires(VariableDictionaryEntryType entry) {
/**
* @return The dictionary Id for this variable.
*/
{
entry.get_id()
} -> std::same_as<variable_dictionary_id_t>;
};

/**
* Requirement for the logtype dictionary reader interface.
* @tparam LogTypeDictionaryReaderType The type of the logtype dictionary reader.
* @tparam LogTypeDictionaryEntryType The type of the entries in the logtype dictionary reader.
*/
template <
typename LogTypeDictionaryReaderType,
typename LogTypeDictionaryEntryType = typename LogTypeDictionaryReaderType::entry_t>
concept LogTypeDictionaryReaderReq = requires(
LogTypeDictionaryReaderType reader,
std::string_view logtype,
bool ignore_case,
std::unordered_set<LogTypeDictionaryEntryType const*>& entries
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider relaxing the concrete container type for entries.

Requiring std::unordered_set<... const*> is unnecessarily restrictive. A lightweight set concept would let callers pass any compatible container (e.g., flat_hash_set, std::pmr::unordered_set).

Example approach:

template <typename Set, typename EntryPtr>
concept EntryPtrSetReq = requires(Set s, EntryPtr p) {
    { s.insert(p) } -> std::same_as<std::pair<typename Set::iterator, bool>>;
    // Optionally require size/clear if needed
};

template <typename LogTypeDictionaryReaderType,
          typename LogTypeDictionaryEntryType = typename LogTypeDictionaryReaderType::entry_t,
          typename EntrySet = std::unordered_set<LogTypeDictionaryEntryType const*>>
concept LogTypeDictionaryReaderReq = requires(
        LogTypeDictionaryReaderType reader,
        std::string_view logtype,
        bool ignore_case,
        EntrySet& entries
) {
    requires EntryPtrSetReq<EntrySet, LogTypeDictionaryEntryType const*>;
    ...
};

If you’d prefer to keep the current signature, at least accept auto& entries and constrain with a nested requires.

Also applies to: 224-225

/**
* Gets entries matching a given logtype.
* @param logtype
* @param ignore_case Whether the lookup should be case insensitive.
* @return A vector of entries matching the given logtype.
*/
{
reader.get_entry_matching_value(logtype, ignore_case)
} -> std::same_as<std::vector<LogTypeDictionaryEntryType const*>>;

/**
* Gets entries matching a wildcard string.
* @param logtype A wildcard search string.
* @param ignore_case Whether the search should be case insensitive.
* @param entries A hash set in which to store the found entries.
*/
{
reader.get_entries_matching_wildcard_string(logtype, ignore_case, entries)
} -> std::same_as<void>;

requires std::
same_as<typename LogTypeDictionaryReaderType::dictionary_id_t, logtype_dictionary_id_t>;

requires std::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation takes a template parameter LogTypeDictionaryEntryType and sets it with a default value, and later we have an assertion to check whether the argument is the default value. I think this gets a bit weird; like if we want them to be the same, then probably we just don't expose it as the default value. Maybe do the following to explicitly use LogTypeDictionaryReader::entry_t:

template <typename LogTypeDictionaryReaderType>
concept LogTypeDictionaryReaderReq = requires(
        LogTypeDictionaryReaderType reader,
        std::string_view logtype,
        bool ignore_case,
        std::unordered_set<typename LogTypeDictionaryReaderType::entry_t const*>& entries
) {
    requires LogTypeDictionaryEntryReq<typename LogTypeDictionaryReaderType::entry_t>;

    /**
     * Gets entries matching a given logtype.
     * @param logtype
     * @param ignore_case Whether the lookup should be case insensitive.
     * @return A vector of entries matching the given logtype.
     */
    {
        reader.get_entry_matching_value(logtype, ignore_case)
    } -> std::same_as<std::vector<typename LogTypeDictionaryReaderType::entry_t const*>>;

    /**
     * Gets entries matching a wildcard string.
     * @param logtype A wildcard search string.
     * @param ignore_case Whether the search should be case insensitive.
     * @param entries A hash set in which to store the found entries.
     */
    {
        reader.get_entries_matching_wildcard_string(logtype, ignore_case, entries)
    } -> std::same_as<void>;

    requires std::
            same_as<typename LogTypeDictionaryReaderType::dictionary_id_t, logtype_dictionary_id_t>;
};

Notice that:

  • We should add a check to ensure the entry type matches the concept requirement.
  • entry_t actually doesn't match our latest naming rule, as it's not a fundamental type. Shall we create an issue to rename it in another PR? (or maybe u can do it in this PR if the change isn't propagating too wild)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I guess I'm fine with changing it to EntryT in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify: I think we should call it Entry. We only use XxxType suffix in a template parameter.

same_as<typename LogTypeDictionaryReaderType::entry_t, LogTypeDictionaryEntryType>;
};

/**
* Requirement for the variable dictionary writer interface.
* @tparam VariableDictionaryWriterType The type of the variable dictionary writer.
*/
template <typename VariableDictionaryWriterType>
concept VariableDictionaryWriterReq = requires(
VariableDictionaryWriterType writer,
std::string_view value,
variable_dictionary_id_t& id_ref
) {
/**
* Adds the given variable to the dictionary if it doesn't exist.
* @param value
* @param id_ref The Id of the variable matching the given entry.
* @return Whether this call resulted in inserting a new entry or not.
*/
{
writer.add_entry(value, id_ref)
} -> std::same_as<bool>;
};

/**
* Requirement for the variable dictionary reader interface.
* @tparam VariableDictionaryReaderType The type of the variable dictionary reader.
* @tparam VariableDictionaryEntryType The type of the entries in the variable dictionary reader.
*/
template <
typename VariableDictionaryReaderType,
typename VariableDictionaryEntryType = typename VariableDictionaryReaderType::entry_t>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the logtype reader:

  • How about using VariableDictionaryReaderType::entry_t directly?
  • We need to check it matches the variable entry requirement.

concept VariableDictionaryReaderReq = requires(
VariableDictionaryReaderType reader,
variable_dictionary_id_t id,
std::string_view variable,
bool ignore_case,
std::unordered_set<VariableDictionaryEntryType const*>& entries
) {
/**
* @param id
* @return The value of the dictionary entry with the given Id.
*/
{
reader.get_value(id)
} -> std::same_as<std::string const&>;

/**
* Gets entries matching a given variable value.
* @param variable The variable value to look up.
* @param ignore_case Whether the lookup should be case insensitive.
* @return A vector of entries matching the given variable value.
*/
{
reader.get_entry_matching_value(variable, ignore_case)
} -> std::same_as<std::vector<VariableDictionaryEntryType const*>>;

/**
* Gets entries matching a given wildcard string.
* @param variable A wildcard search string.
* @param ignore_case Whether the search should be case insensitive.
* @param entries A hash set in which to store the found entries.
*/
{
reader.get_entries_matching_wildcard_string(variable, ignore_case, entries)
} -> std::same_as<void>;

requires std::same_as<
typename VariableDictionaryReaderType::dictionary_id_t,
variable_dictionary_id_t>;

requires std::
same_as<typename VariableDictionaryReaderType::entry_t, VariableDictionaryEntryType>;
};
} // namespace clp

#endif // CLP_DICTIONARYCONCEPTS_HPP
Loading
Loading