Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(clp-s): Replace instances of std::string const& with std::string_view where it would remove unnecessary conversions to and from std::string. #635

Merged
merged 5 commits into from
Dec 16, 2024
Merged

refactor(clp-s): Replace instances of std::string const& with std::string_view where it would remove unnecessary conversions to and from std::string. #635

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d62e5e
Refactor many clp_s components to use string_view, and remove some du…
gibber9809 Dec 13, 2024
896fe91
Apply suggestions from code review
gibber9809 Dec 13, 2024
dd1539d
Address review comments
gibber9809 Dec 13, 2024
22e5ad4
Address review comments
gibber9809 Dec 13, 2024
c993273
Add missed string_view in ArchiveWriter
gibber9809 Dec 16, 2024
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
2 changes: 1 addition & 1 deletion components/core/src/clp_s/ArchiveWriter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ class ArchiveWriter {
epochtime_t ingest_timestamp_entry(
std::string const& key,
int32_t node_id,
std::string const& timestamp,
std::string_view timestamp,
uint64_t& pattern_id
) {
return m_timestamp_dict.ingest_entry(key, node_id, timestamp, pattern_id);
Expand Down
10 changes: 10 additions & 0 deletions components/core/src/clp_s/ParsedMessage.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
#ifndef CLP_S_PARSEDMESSAGE_HPP
#define CLP_S_PARSEDMESSAGE_HPP

#include <cstdint>
#include <map>
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
#include <string>
#include <string_view>
#include <utility>
#include <variant>

Expand Down Expand Up @@ -34,6 +36,10 @@ class ParsedMessage {
m_message.emplace(node_id, value);
}

inline void add_value(int32_t node_id, std::string_view value) {
m_message.emplace(node_id, std::string{value});
}

/**
* Adds a timestamp value and its encoding to the message for a given MST node ID.
* @param node_id
Expand All @@ -55,6 +61,10 @@ class ParsedMessage {
m_unordered_message.emplace_back(value);
}

inline void add_unordered_value(std::string_view value) {
m_unordered_message.emplace_back(std::string{value});
}

/**
* Clears the message
*/
Expand Down
10 changes: 6 additions & 4 deletions components/core/src/clp_s/TimestampDictionaryWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#include "TimestampDictionaryWriter.hpp"

#include <cstdint>
#include <sstream>
#include <string_view>

#include "Utils.hpp"

Expand Down Expand Up @@ -42,9 +44,9 @@ uint64_t TimestampDictionaryWriter::get_pattern_id(TimestampPattern const* patte
}

epochtime_t TimestampDictionaryWriter::ingest_entry(
std::string const& key,
std::string_view key,
int32_t node_id,
std::string const& timestamp,
std::string_view timestamp,
uint64_t& pattern_id
) {
epochtime_t ret;
Expand Down Expand Up @@ -88,7 +90,7 @@ epochtime_t TimestampDictionaryWriter::ingest_entry(
}

void TimestampDictionaryWriter::ingest_entry(
std::string const& key,
std::string_view key,
int32_t node_id,
double timestamp
) {
Expand All @@ -103,7 +105,7 @@ void TimestampDictionaryWriter::ingest_entry(
}

void TimestampDictionaryWriter::ingest_entry(
std::string const& key,
std::string_view key,
int32_t node_id,
int64_t timestamp
) {
Expand Down
10 changes: 6 additions & 4 deletions components/core/src/clp_s/TimestampDictionaryWriter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
#ifndef CLP_S_TIMESTAMPDICTIONARYWRITER_HPP
#define CLP_S_TIMESTAMPDICTIONARYWRITER_HPP

#include <cstdint>
#include <map>
#include <sstream>
#include <string>
#include <string_view>
#include <unordered_map>
#include <utility>

Expand Down Expand Up @@ -47,9 +49,9 @@ class TimestampDictionaryWriter {
* @return the epoch time corresponding to the string timestamp
*/
epochtime_t ingest_entry(
Copy link
Member

Choose a reason for hiding this comment

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

Not mandatory, but since it seems like a small change, do you want to change the callers in ArchiveWriter to also use string_views?

std::string const& key,
std::string_view key,
int32_t node_id,
std::string const& timestamp,
std::string_view timestamp,
uint64_t& pattern_id
);

Expand All @@ -59,9 +61,9 @@ class TimestampDictionaryWriter {
* @param node_id
* @param timestamp
*/
void ingest_entry(std::string const& key, int32_t node_id, double timestamp);
void ingest_entry(std::string_view key, int32_t node_id, double timestamp);

void ingest_entry(std::string const& key, int32_t node_id, int64_t timestamp);
void ingest_entry(std::string_view key, int32_t node_id, int64_t timestamp);

/**
* TODO: guarantee epoch milliseconds. The current clp-s approach to encoding timestamps and
Expand Down
3 changes: 2 additions & 1 deletion components/core/src/clp_s/TimestampEntry.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include <sstream>
#include <string>
#include <string_view>
#include <unordered_set>
#include <variant>

Expand Down Expand Up @@ -43,7 +44,7 @@ class TimestampEntry {
m_epoch_start(cEpochTimeMax),
m_epoch_end(cEpochTimeMin) {}

TimestampEntry(std::string const& key_name)
TimestampEntry(std::string_view key_name)
: m_encoding(UnkownTimestampEncoding),
m_epoch_start_double(cDoubleEpochTimeMax),
m_epoch_end_double(cDoubleEpochTimeMin),
Expand Down
14 changes: 8 additions & 6 deletions components/core/src/clp_s/TimestampPattern.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include <chrono>
#include <cstring>
#include <string>
#include <string_view>
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
#include <vector>

#include <date/include/date/date.h>
Expand Down Expand Up @@ -71,7 +73,7 @@ append_padded_value_notz(int value, char padding_character, size_t max_length, s
* @return true if conversion succeeds, false otherwise
*/
static bool convert_string_to_number(
string const& str,
std::string_view str,
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I missed this one.
nit: this file has been using "using std::string". how about we do the same for std::string_view, so we get rid of the std:: and it's consistent with the rest of file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking a bit more at the file it looks like its already a bit inconsistent with using std:: even for things it declared as using. I'll go through and remove all of the using to make things consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually on second though it mostly isn't using std::, so I'll just change the few other exceptions to be consistent.

size_t begin_ix,
size_t end_ix,
char padding_character,
Expand All @@ -89,7 +91,7 @@ static bool convert_string_to_number(
* @return true if conversion succeeds, false otherwise
*/
static bool convert_string_to_number_notz(
string const& str,
std::string_view str,
size_t max_digits,
size_t begin_ix,
size_t& end_ix,
Expand Down Expand Up @@ -125,7 +127,7 @@ append_padded_value_notz(int value, char padding_character, size_t max_length, s
}

static bool convert_string_to_number(
string const& str,
std::string_view str,
size_t begin_ix,
size_t end_ix,
char padding_character,
Expand Down Expand Up @@ -154,7 +156,7 @@ static bool convert_string_to_number(
}

static bool convert_string_to_number_notz(
string const& str,
std::string_view str,
size_t max_digits,
size_t begin_ix,
size_t& end_ix,
Expand Down Expand Up @@ -306,7 +308,7 @@ void TimestampPattern::init() {
}

TimestampPattern const* TimestampPattern::search_known_ts_patterns(
string const& line,
std::string_view line,
epochtime_t& timestamp,
size_t& timestamp_begin_pos,
size_t& timestamp_end_pos
Expand Down Expand Up @@ -342,7 +344,7 @@ void TimestampPattern::clear() {
}

bool TimestampPattern::parse_timestamp(
string const& line,
std::string_view line,
epochtime_t& timestamp,
size_t& timestamp_begin_pos,
size_t& timestamp_end_pos
Expand Down
6 changes: 4 additions & 2 deletions components/core/src/clp_s/TimestampPattern.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#include <cstddef>
#include <cstdint>
#include <memory>
#include <string>
#include <string_view>
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
#include <utility>

#include "Defs.hpp"
Expand Down Expand Up @@ -83,7 +85,7 @@ class TimestampPattern {
* @return pointer to the timestamp pattern if found, nullptr otherwise
*/
static TimestampPattern const* search_known_ts_patterns(
std::string const& line,
std::string_view line,
epochtime_t& timestamp,
size_t& timestamp_begin_pos,
size_t& timestamp_end_pos
Expand Down Expand Up @@ -121,7 +123,7 @@ class TimestampPattern {
* @return true if parsed successfully, false otherwise
*/
bool parse_timestamp(
std::string const& line,
std::string_view line,
epochtime_t& timestamp,
size_t& timestamp_begin_pos,
size_t& timestamp_end_pos
Expand Down
16 changes: 3 additions & 13 deletions components/core/src/clp_s/search/StringLiteral.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <memory>
#include <string>

#include "../Utils.hpp"
#include "Literal.hpp"

namespace clp_s::search {
Expand Down Expand Up @@ -68,19 +69,8 @@ class StringLiteral : public Literal {
m_string_type = LiteralType::VarStringT;
}

// If '?' and '*' are not escaped, we add LiteralType::ClpStringT to m_string_type
bool escape = false;
for (char const c : m_v) {
if ('\\' == c) {
escape = !escape;
} else if ('?' == c || '*' == c) {
if (false == escape) {
m_string_type |= LiteralType::ClpStringT;
break;
}
} else {
escape = false;
}
if (StringUtils::has_unescaped_wildcards(m_v)) {
m_string_type |= LiteralType::ClpStringT;
}
}
};
Expand Down
Loading