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
Conversation
WalkthroughThis pull request introduces modifications across several classes to change method parameter types from Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
components/core/src/clp_s/TimestampDictionaryWriter.cpp (1)
Line range hint
47-91: Consider updating nullptr check to match coding guidelines.The check
nullptr == patternfollows the coding guideline of preferringfalse == <expression>style, but could be more consistent throughout the function.Apply this change:
- if (nullptr == pattern) { + if (nullptr == pattern) { pattern = TimestampPattern::search_known_ts_patterns( timestamp, ret, timestamp_begin_pos, timestamp_end_pos ); pattern_id = get_pattern_id(pattern); } - if (nullptr == pattern) { + if (nullptr == pattern) { throw OperationFailed(ErrorCodeFailure, __FILE__, __LINE__); }components/core/src/clp_s/ArchiveWriter.hpp (1)
127-127: Consider using string_view for the key parameter as well.While the change to use
string_view constfor the timestamp parameter is good, consider extending this optimization to thekeyparameter as well, since it's also used as a read-only value.- epochtime_t ingest_timestamp_entry( - std::string const& key, - int32_t node_id, - std::string_view const timestamp, - uint64_t& pattern_id - ) + epochtime_t ingest_timestamp_entry( + std::string_view const key, + int32_t node_id, + std::string_view const timestamp, + uint64_t& pattern_id + )components/core/src/clp_s/search/StringLiteral.hpp (1)
73-74: Excellent refactoring to use the utility functionThe replacement of manual wildcard checking with
StringUtils::has_unescaped_wildcardsimproves code maintainability and reduces duplication. The functionality remains unchanged while the implementation is cleaner.Consider adding a brief comment explaining why
ClpStringTis added when wildcards are present, as this business logic might not be immediately obvious to new developers:// If '?' and '*' are not escaped, we add LiteralType::ClpStringT to m_string_type + // Wildcards are only supported in CLP strings, so we must include this type if (StringUtils::has_unescaped_wildcards(m_v)) { m_string_type |= LiteralType::ClpStringT; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/core/src/clp_s/ArchiveWriter.hpp(1 hunks)components/core/src/clp_s/ParsedMessage.hpp(3 hunks)components/core/src/clp_s/TimestampDictionaryWriter.cpp(4 hunks)components/core/src/clp_s/TimestampDictionaryWriter.hpp(3 hunks)components/core/src/clp_s/TimestampEntry.hpp(2 hunks)components/core/src/clp_s/TimestampPattern.cpp(7 hunks)components/core/src/clp_s/TimestampPattern.hpp(3 hunks)components/core/src/clp_s/search/StringLiteral.hpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
components/core/src/clp_s/search/StringLiteral.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/TimestampEntry.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/ArchiveWriter.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/ParsedMessage.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/TimestampPattern.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/TimestampDictionaryWriter.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/TimestampDictionaryWriter.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/TimestampPattern.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🔇 Additional comments (17)
components/core/src/clp_s/TimestampPattern.cpp (5)
7-7: LGTM: Required header for string_view support.
The addition of the <string_view> header is necessary and correctly placed in the include section.
75-75: LGTM: Efficient string_view usage for read-only operations.
The change to std::string_view const is appropriate here as the method only needs read access to the string data. This change can improve performance by avoiding unnecessary string copies.
93-93: LGTM: Consistent string_view usage.
The change maintains consistency with other method signatures and is appropriate for the read-only string operations performed.
310-310: LGTM: Appropriate use of string_view for pattern searching.
The change to std::string_view is well-suited for this search operation as it provides efficient read-only access to the string data.
346-346: LGTM: Efficient string handling for timestamp parsing.
The use of std::string_view here is particularly beneficial as it can improve performance in the numerous string comparison operations performed during timestamp parsing.
components/core/src/clp_s/ParsedMessage.hpp (2)
6-6: LGTM: Required header added
The addition of the string_view header is necessary for the new overloads.
38-40: LGTM: Proper string_view handling
The implementation correctly creates owned std::string objects from string_view parameters before storing them in containers. This is necessary since string_view doesn't own its data and the referenced string must outlive the view.
Also applies to: 63-65
components/core/src/clp_s/TimestampDictionaryWriter.hpp (2)
4-4: LGTM: Required headers added
The addition of cstdint and string_view headers is appropriate for the types used in the class.
Also applies to: 8-8
52-54: LGTM: Consistent string_view usage
The method signatures have been updated to use string_view consistently across all overloads. This change aligns with the PR objective of optimizing string handling.
Also applies to: 64-64, 66-66
components/core/src/clp_s/TimestampEntry.hpp (2)
6-6: LGTM: Required header added
The addition of the string_view header is necessary for the constructor parameter.
Line range hint 47-53: LGTM: Proper string ownership handling
The constructor correctly initializes m_key_name as a std::string from the string_view parameter, ensuring proper ownership of the string data.
components/core/src/clp_s/TimestampDictionaryWriter.cpp (3)
3-5: LGTM! Appropriate header includes added.
The addition of <cstdint> and <string_view> headers is correct and necessary for the refactoring.
Line range hint 93-106: LGTM! Consistent parameter type usage.
The function correctly uses string_view const for the key parameter while maintaining the original logic for double timestamp handling.
Line range hint 108-121: LGTM! Consistent parameter type usage.
The function correctly uses string_view const for the key parameter while maintaining the original logic for int64_t timestamp handling.
components/core/src/clp_s/TimestampPattern.hpp (2)
Line range hint 9-91: LGTM! Appropriate header inclusion and parameter type update.
The addition of <string_view> header and the parameter type change to string_view const in search_known_ts_patterns is consistent with the refactoring objective.
Line range hint 125-129: LGTM! Consistent parameter type update.
The parameter type change to string_view const in parse_timestamp is consistent with the refactoring objective.
components/core/src/clp_s/search/StringLiteral.hpp (1)
7-7: Verify the Utils.hpp header inclusion
The addition of this header appears correct for accessing StringUtils::has_unescaped_wildcards, but let's verify its existence and accessibility.
✅ Verification successful
Include directive is correct and properly accessible
The Utils.hpp header exists at the expected relative path (../Utils.hpp from components/core/src/clp_s/search/StringLiteral.hpp), and it contains the required has_unescaped_wildcards function within the StringUtils namespace. The inclusion is appropriate for the refactoring changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of Utils.hpp
# Expected: Find the header file and confirm it contains the required function
# Check if the file exists
fd -t f "Utils.hpp" -x echo "Found: {}"
# Verify the function exists in the file
ast-grep --pattern 'namespace StringUtils {
$$$
bool has_unescaped_wildcards($_) {
$$$
}
$$$
}'
Length of output: 389
Script:
#!/bin/bash
# Let's check the content of the Utils.hpp file in the clp_s directory since that's the one referenced in the relative path
cat ./components/core/src/clp_s/Utils.hpp
# Also check for any references to has_unescaped_wildcards in this file
rg "has_unescaped_wildcards" ./components/core/src/clp_s/Utils.hpp
Length of output: 10127
| @@ -69,18 +70,8 @@ class StringLiteral : public Literal { | |||
| } | |||
|
|
|||
| // If '?' and '*' are not escaped, we add LiteralType::ClpStringT to m_string_type | |||
There was a problem hiding this comment.
I am personally fine with having this comment since I love reading, lol
But since we were talking about comments, what's your opinion? @kirkrodrigues
There was a problem hiding this comment.
When I read the code, I get the same message as the comment (without a loss of speed), so I would drop this comment. Also, this is a minor in this case, but it's possible one day we introduce a new wildcard, making this comment inaccurate (and it's hard to reliably track down these types of inaccurate comments).
| @@ -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 const timestamp, | |||
There was a problem hiding this comment.
for string_view, I believe we don't need the extra const at end.
@LinZhihao-723 what's our standard?
There was a problem hiding this comment.
True, we don't need to add const since the view is immutable
Co-authored-by: haiqi96 <14502009+haiqi96@users.noreply.github.com>
gibber9809
changed the title
| @@ -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, | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actually on second though it mostly isn't using std::, so I'll just change the few other exceptions to be consistent.
|
Following https://github.com/commitizen/conventional-commit-types/blob/master/index.json. chore refers to "Other changes that don't modify src or test files". For this PR, I believe we should use "refactor" |
gibber9809
changed the title
|
Looks good to me. |
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. @haiqi96: Here are some suggestions for the PR title:
These titles highlight the core changes and the performance benefits of using |
| @@ -47,9 +49,9 @@ class TimestampDictionaryWriter { | |||
| * @return the epoch time corresponding to the string timestamp | |||
| */ | |||
| epochtime_t ingest_entry( | |||
There was a problem hiding this comment.
Not mandatory, but since it seems like a small change, do you want to change the callers in ArchiveWriter to also use string_views?
|
macOS build failure is unrelated to this PR and should be resolved by #636. |
gibber9809
changed the title
std::string const& with std::string_view where it would remove unnecessary conversions to and from std::string.
Description
This PR splits out some of the refactoring work from the escape character fix PR to ease review.
In particular this PR:
This PR is pure refactoring with no functional changes.
Validation performed
Summary by CodeRabbit
New Features
std::string_viewacross multiple classes and methods.Bug Fixes
StringLiteralclass for detecting unescaped wildcard characters.Documentation