fix: String2int error #58
Conversation
WalkthroughThe changes introduce new functions for stricter string-to-integer conversions in the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🔇 Additional comments (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 (
|
hahahashen
force-pushed
the
fix/hincrby
branch
from
26a9280
to
f418bef
Compare
![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 (1)
src/pstd/pstd_string.h (1)
63-67: Approve changes with a minor suggestion for improvementThe modifications to the
String2intfunction effectively address the issue mentioned in the PR objectives. The new implementation correctly ensures that the entire string is parsed, fixing the problem where strings like "11 " were incorrectly considered valid conversions.The logic is now more accurate and aligns with the expected behavior of string-to-integer conversion.
Consider simplifying the return statement for better readability:
- if (ptr==s+slen) { - return 1; - } else { - return 0; - } + return (ptr == s + slen) ? 1 : 0;This change maintains the same logic while making the code more concise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/pstd/pstd_string.h (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/pstd/pstd_string.h (1)
63-67: Verify impact on existing codeThe changes to the
String2intfunction improve its accuracy but may affect existing code that relies on the previous behavior.To ensure a smooth transition, please run the following script to identify potential areas that might be affected by this change:
Review the output to identify any code that might need adjustment due to the changed behavior of
String2int.
hahahashen
force-pushed
the
fix/hincrby
branch
from
f418bef
to
534e913
Compare
src/pstd/pstd_string.h
Outdated
| if (ec != std::errc()) { | ||
| return 0; | ||
| } else { | ||
| if (ptr == s + slen) { |
There was a problem hiding this comment.
特殊 case：
"123 "
期望：false
实际：true
There was a problem hiding this comment.
string aa = "123 "
if(aa.trim().length != aa.length) {
return false;
}
There was a problem hiding this comment.
template <std::integral T>
int String2intStrict(const char* s, size_t slen, T* val) {
auto [ptr, ec] = std::from_chars(s, s + slen, *val);
if (ec != std::errc()) {
return 0;
}
if (ptr != s + slen) {
return 0;
}
return 1;
}
如果期望 "123 " 这样的字符串是非法的, 可以加一个严格模式下的 string 2 int 函数
hahahashen
force-pushed
the
fix/hincrby
branch
from
534e913
to
00ca779
Compare
hahahashen
force-pushed
the
fix/hincrby
branch
from
00ca779
to
4079e97
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
src/storage/include/storage/util.h (1)
20-20: LGTM! Consider adding a brief comment for clarity.The addition of
StrToInt64Strictis well-placed and aligns with the PR objectives. It provides a stricter alternative toStrToInt64, which should address the issue with partial string matches.Consider adding a brief inline comment to clarify the difference between this function and
StrToInt64. For example:// Converts string to int64 with strict validation (entire string must match) int StrToInt64Strict(const char* s, size_t slen, int64_t* value);src/pstd/pstd_string.h (1)
70-83: Excellent addition ofString2intStrictfunction.The new
String2intStrictfunction addresses the issue raised in previous comments and provides a stricter string-to-integer conversion. The implementation is correct and efficient, usingstd::from_charsand checking for complete string consumption.Consider adding a brief example in the function documentation to illustrate the difference between
String2intandString2intStrict:/* Convert a string into an integral. Returns 1 if the entire string could be parsed * into an integer, 0 otherwise. The value will be set to * the parsed value when appropriate. * * Example: * String2int("123", 3, &val) returns 1 (val = 123) * String2int("123 ", 4, &val) returns 1 (val = 123) * String2intStrict("123", 3, &val) returns 1 (val = 123) * String2intStrict("123 ", 4, &val) returns 0 (invalid) */This addition would help developers quickly understand the difference between the two functions.
src/storage/src/util.cc (1)
40-43: LGTM! Consider clarifying the comment.The new
StrToInt64Strictfunction looks good and addresses the issue mentioned in the PR objectives. It provides a stricter conversion method, which should prevent incorrect conversions like "11 " that were problematic before.Consider updating the comment to clarify the difference between this function and
StrToInt64:-/* Convert a string into a long long. Returns 1 if all char of string could be parsed +/* Convert a string into a long long. Returns 1 if and only if all characters of the string could be parsed * into a (non-overflowing) long long, 0 otherwise. The value will be set to - * the parsed value when appropriate. */ + * the parsed value when successful. This function is stricter than StrToInt64. */src/storage/src/redis_hashes.cc (6)
Line range hint
311-313: Ensure proper overflow checks when incrementing integersThe current overflow check in the condition:
if ((value >= 0 && LLONG_MAX - value < ival) || (value < 0 && LLONG_MIN - value > ival)) { return Status::InvalidArgument("Overflow"); }may lead to incorrect results due to potential integer overflows during the calculation
LLONG_MAX - valueandLLONG_MIN - value. Consider rearranging the overflow checks to prevent this issue.Apply this diff to correct the overflow checks:
- if ((value >= 0 && LLONG_MAX - value < ival) || (value < 0 && LLONG_MIN - value > ival)) { + if ((value > 0 && ival > LLONG_MAX - value) || (value < 0 && ival < LLONG_MIN - value)) {This adjustment ensures that the subtraction does not cause an overflow itself.
Line range hint
327-329: Return appropriate status for invalid float inputWhen
StrToLongDoublefails to parsebyas a valid floating-point number (returns-1), the function returnsStatus::Corruption("value is not a valid float"). It's more appropriate to useStatus::InvalidArgument("value is not a valid float")to indicate invalid input from the user.Apply this diff to correct the error status:
- return Status::Corruption("value is not a valid float"); + return Status::InvalidArgument("value is not a valid float");
Line range hint
345-347: Handle invalid stored float values appropriatelyIf the existing hash field's value cannot be parsed as a float by
StrToLongDouble(returns-1), the code returnsStatus::Corruption("value is not a valid float"). Since this indicates invalid data rather than data corruption, consider returningStatus::InvalidArgument("hash value is not a valid float")instead.Apply this diff to adjust the error status:
- return Status::Corruption("value is not a valid float"); + return Status::InvalidArgument("hash value is not a valid float");
Line range hint
690-695: Replacerand()with C++11<random>facilities for better randomnessThe use of
rand()for generating random indices may result in poor randomness and is not thread-safe. It's recommended to use the C++11<random>library for improved randomness and thread safety.Apply this diff to use
std::mt19937andstd::uniform_int_distribution:- idxs.push_back(rand() % hlen); + { + static thread_local std::mt19937 generator(std::random_device{}()); + std::uniform_int_distribution<uint32_t> distribution(0, hlen - 1); + idxs.push_back(distribution(generator)); + }Repeat this change in the loop below:
- while (idxs.size() < -count) { - idxs.push_back(rand() % hlen); + while (idxs.size() < static_cast<size_t>(-count)) { + static thread_local std::mt19937 generator(std::random_device{}()); + std::uniform_int_distribution<uint32_t> distribution(0, hlen - 1); + idxs.push_back(distribution(generator)); + }
Line range hint
741-743: Check for division by zero inHRandFieldIn the calculation of
distribution(0, hlen - 1), ifhlenis zero, this will result in an invalid distribution range. Ensure thathlenis greater than zero before using it to prevent potential runtime errors.Apply this diff to add a check for
hlen == 0:+ if (hlen == 0) { + return Status::NotFound("Hash is empty"); + }
Line range hint
212-295: Consider code reuse betweenHIncrbyandHIncrbyfloatBoth
HIncrbyandHIncrbyfloatfunctions have similar logic for handling increments. Refactoring common code into a shared helper function can improve maintainability and reduce code duplication.
🛑 Comments failed to post (1)
src/storage/src/redis_hashes.cc (1)
293-295:
⚠️ Potential issueUse appropriate error status for invalid integer conversion
When
StrToInt64Strictfails to convertold_valueto an integer (returns0), the function currently returnsStatus::Corruption("hash value is not an integer"). For invalid input errors, it's more suitable to returnStatus::InvalidArgument("hash value is not an integer")to indicate that the provided data is invalid rather than corrupted.Apply this diff to correct the error status:
- return Status::Corruption("hash value is not an integer"); + return Status::InvalidArgument("hash value is not an integer");📝 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.if (StrToInt64Strict(old_value.data(), old_value.size(), &ival) == 0) { return Status::InvalidArgument("hash value is not an integer"); }
解决issue #55
原来的String2int判断逻辑是通过错误码来进行判断，但是根据函数解释，"11 "会被认为转换成功，于是修改判断逻辑，改为是否都匹配上了，即ptr与s+slen的比较
修改后测试通过
Summary by CodeRabbit
Summary by CodeRabbit