feat:variable-length arguments in configuration(closes #146) #164
Conversation
|
Warning Rate limit exceeded@wxnzb has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Poem
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
🧹 Nitpick comments (1)
src/config.cc (1)
77-78: LGTM with suggestions for improvement.The change correctly implements variable-length argument support by allowing more parameters than required while still catching cases with insufficient parameters. However, consider these improvements:
- Add a warning when extra parameters are provided to help users identify potential configuration mistakes.
- Consider adding bounds checking in the subsequent loop to prevent potential buffer overflows.
Here's a suggested improvement:
if (values.size() < values_.size()) { return Status::InvalidArgument("The number of parameters is less than required."); + } else if (values.size() > values_.size()) { + // Log a warning about extra parameters being ignored } - } else { // if the value_ is empty, resize the value_ to the size of the values + } else { values_.resize(values.size()); } - for (int i = 0; i < values.size(); i++) { + // Only process up to values_.size() elements to prevent buffer overflows + for (int i = 0; i < values_.size(); i++) { values_[i] = std::move(values[i]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/config.cc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_macos
- GitHub Check: build_on_ubuntu
🔇 Additional comments (1)
src/config.cc (1)
77-78: Verify the impact of variable-length arguments support.Let's analyze the usage patterns and test coverage for this change.
✅ Verification successful
Parameter validation is correctly implemented and necessary
The validation check prevents incomplete parameter sets from being applied through the configuration system, which is essential because:
- The configuration can be modified at runtime through admin commands
- The system expects complete parameter sets for proper functionality
- It maintains configuration integrity across all supported options (boolean, string, and array values)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the usage of StringValueArray and configuration parsing # Find all StringValueArray usages echo "Finding StringValueArray usages:" rg "StringValueArray" -A 5 # Find configuration parsing patterns echo -e "\nAnalyzing configuration parsing patterns:" rg "SplitString.*delimiter_" -A 5 # Look for related tests echo -e "\nLooking for related tests:" fd "test.*\.cc" | xargs rg "StringValueArray|SetValue.*values"Length of output: 2303
Script:
#!/bin/bash # Search for test files and configuration examples echo "Looking for test files:" fd -e cc -e cpp -e h "test" | xargs rg -l "Config|config" echo -e "\nLooking for configuration examples:" fd "config" -e conf -e ini -e yaml -e json echo -e "\nSearching for documentation:" fd "README|CONTRIBUTING" -e md | xargs rg -l "config|configuration" -i echo -e "\nAnalyzing Config class usage:" rg "Config::" -A 3Length of output: 2509
src/config.cc
Outdated
| @@ -74,8 +74,8 @@ Status StringValue::SetValue(const std::string& value) { | |||
| Status StringValueArray::SetValue(const std::string& value) { | |||
| auto values = SplitString(value, delimiter_); | |||
| if (!values_.empty()) { // if the value_ is not empty, check the number of parameters | |||
| if (values.size() != values_.size()) { | |||
| return Status::InvalidArgument("The number of parameters does not match."); | |||
| if (values.size() < values_.size()) { | |||
There was a problem hiding this comment.
需要支持两种情况
- 需要校验默认参数和实际参数长度是否一致
- 不需要校验默认参数长度
可以在SetValue(const std::string& value) 加一个默认参数, 是否需要校验默认参数
比如
SetValue(const std::string& value, bool check = false)大多数情况下,应该都不需要检查默认参数长度,所以默认参数用 false 即可
有一种情况需要注意, 配置的默认参数填了2个, 但 配置文件中填了一个,需要把默认的两个都覆盖了,只用配置文件中的一个
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/config.cc(1 hunks)src/config.h(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: kiwi
src/config.cc
[error] 81-81: Clang format style issue: Extra whitespace at the end of line after values.size()
🔇 Additional comments (2)
src/config.h (1)
87-88: LGTM! Well-designed method signature.The new
SetValueoverload with thecheckparameter provides a clean way to support both validation scenarios for variable-length arguments.src/config.cc (1)
74-75: LGTM! Good default behavior.The implementation correctly delegates to the new method with
check=false, maintaining backward compatibility while defaulting to the more flexible behavior.
src/config.cc
Outdated
| Status StringValueArray::SetValue(const std::string& value, bool check) { | ||
| auto values = SplitString(value, delimiter_); | ||
| if (!values_.empty()) { // if the value_ is not empty, check the number of parameters | ||
| if (check) { | ||
| if (values.size() != values_.size()) { | ||
| return Status::InvalidArgument("The number of parameters does not match."); | ||
| } | ||
| } else { // if the value_ is empty, resize the value_ to the size of the values | ||
| values_.resize(values.size()); | ||
| } | ||
|
|
||
| for (int i = 0; i < values.size(); i++) { | ||
| values_[i] = std::move(values[i]); | ||
| } else { | ||
| if (values_.empty()) { | ||
| values_.resize(values.size()); | ||
| } | ||
| for (size_t i = 0; i < values.size(); i++) { | ||
| values_[i] = std::move(values[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix formatting and improve error handling.
The implementation has a few issues that need attention:
- There's an extra whitespace at the end of line 81 (as reported by the pipeline).
- The logic in the
elsebranch could be improved to handle the case wherevalues_.size()is less thanvalues.size().
Apply this diff to fix the issues:
Status StringValueArray::SetValue(const std::string& value, bool check) {
auto values = SplitString(value, delimiter_);
if (check) {
- if (values.size() != values_.size()) {
+ if (values.size() != values_.size()) {
return Status::InvalidArgument("The number of parameters does not match.");
}
} else {
- if (values_.empty()) {
- values_.resize(values.size());
- }
+ values_.resize(values.size());
for (size_t i = 0; i < values.size(); i++) {
values_[i] = std::move(values[i]);
}
}
return Status::OK();
}The changes:
- Remove extra whitespace after
values.size() - Always resize
values_in the non-check branch to ensure it can accommodate all values - Use
std::moveto avoid unnecessary copies (already implemented correctly)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Status StringValueArray::SetValue(const std::string& value, bool check) { | |
| auto values = SplitString(value, delimiter_); | |
| if (!values_.empty()) { // if the value_ is not empty, check the number of parameters | |
| if (check) { | |
| if (values.size() != values_.size()) { | |
| return Status::InvalidArgument("The number of parameters does not match."); | |
| } | |
| } else { // if the value_ is empty, resize the value_ to the size of the values | |
| values_.resize(values.size()); | |
| } | |
| for (int i = 0; i < values.size(); i++) { | |
| values_[i] = std::move(values[i]); | |
| } else { | |
| if (values_.empty()) { | |
| values_.resize(values.size()); | |
| } | |
| for (size_t i = 0; i < values.size(); i++) { | |
| values_[i] = std::move(values[i]); | |
| } | |
| } | |
| Status StringValueArray::SetValue(const std::string& value, bool check) { | |
| auto values = SplitString(value, delimiter_); | |
| if (check) { | |
| if (values.size() != values_.size()) { | |
| return Status::InvalidArgument("The number of parameters does not match."); | |
| } | |
| } else { | |
| values_.resize(values.size()); | |
| for (size_t i = 0; i < values.size(); i++) { | |
| values_[i] = std::move(values[i]); | |
| } | |
| } | |
| return Status::OK(); | |
| } |
🧰 Tools
🪛 GitHub Actions: kiwi
[error] 81-81: Clang format style issue: Extra whitespace at the end of line after values.size()
| if (values_.empty()) { | ||
| values_.resize(values.size()); | ||
| } | ||
| for (size_t i = 0; i < values.size(); i++) { |
There was a problem hiding this comment.
这里应该在 value和value_ 长度不相等就需要调用 values_.resize(values.size());
比如有一种情况, value_的默认值是 ["config1", "config2", "config3"], 但是在配置文件中的配置是
["config_1", "config_2"] 如果这时不调用 resize 函数, 那么读取配置后, 内存中的实际配置是
["config_1", "config_2", "config3"] 不符合预期的
在配置中添加对不定长参数的支持
Summary by CodeRabbit
Summary by CodeRabbit
fix #146
StringValueArrayclass with a new method for setting values, enhancing its functionality.