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

fix: Fix Ctest on String #1782

Merged
merged 1 commit into from
Jul 21, 2023
Merged

fix: Fix Ctest on String #1782

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions src/storage/src/redis_strings.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,11 +468,11 @@ Status RedisStrings::Incrby(const Slice& key, int64_t value, int64_t* ret) {
std::string new_value;
ScopeRecordLock l(lock_mgr_, key);
Status s = db_->Get(default_read_options_, key, &old_value);
char buf[32] = {0};
if (s.ok()) {
ParsedStringsValue parsed_strings_value(&old_value);
if (parsed_strings_value.IsStale()) {
*ret = value;
char buf[32];
Int64ToStr(buf, 32, value);
StringsValue strings_value(buf);
return db_->Put(default_write_options_, key, strings_value.Encode());
Expand All @@ -495,7 +495,6 @@ Status RedisStrings::Incrby(const Slice& key, int64_t value, int64_t* ret) {
}
} else if (s.IsNotFound()) {
*ret = value;
char buf[32];
Int64ToStr(buf, 32, value);
StringsValue strings_value(buf);
return db_->Put(default_write_options_, key, strings_value.Encode());
Copy link

Choose a reason for hiding this comment

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

Here are some observations and suggestions for the code patch:

  1. The code initializes buf to zero at the beginning of the function: char buf[32] = {0};. Later, there is another initialization of buf without the zeroing: char buf[32];. It would be cleaner to remove the first initialization since it is not needed.

  2. Considering that the variable buf is used multiple times within the function, it might be better to declare it at the top of the block, before the if (s.ok()) condition. This way, you don't need to repeat its declaration in each branch of the if statement.

  3. In the else-if block (else if (s.IsNotFound())), the code repeats the same logic for setting the value of buf and creating a StringsValue object. You can avoid this repetition by moving those lines outside the if-else block and using them after the conditional branches.

  4. It's important to verify if the integer-to-string conversion Int64ToStr will always produce a string representation that fits within the 32-character buffer. If there is a possibility of the converted string exceeding 32 characters, it could lead to buffer overflow issues. Make sure to handle such cases appropriately.

  5. Consider adding error handling for the database Put operation returns an error. It's good practice to handle errors and propagate them upwards so that the caller can be aware of any failures.

Overall, the code patch seems reasonable with these suggested improvements. However, a comprehensive review would require more context and understanding of the surrounding code and requirements.

Copy link

Choose a reason for hiding this comment

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

Here are some observations and recommendations for the code patch you provided:

  1. Redundant variable declaration: The buf array is declared twice, once before the if (s.ok()) block and again before the else if (s.IsNotFound()) block. You can remove the first declaration since it's redundant.

  2. Magic number: Instead of using the magic number 32 directly in Int64ToStr(buf, 32, value), it would be better to define a named constant for it or use a safer alternative like snprintf if available.

  3. Error handling: It is advisable to handle potential errors when creating and encoding StringsValue. Consider validating the return status of strings_value.Encode() to ensure successful encoding before performing the database put operation.

  4. Code duplication: There is code duplication in both branches of the if (s.ok()) block and the else if (s.IsNotFound()) block. You can refactor the code to eliminate this duplication, reducing the risk of introducing bugs and making the code more maintainable.

  5. Variable scope optimization: Consider moving variable declarations inside their respective scope blocks instead of declaring them at the beginning of the function. This can improve code readability and reduce the possibility of mistakenly reusing variables in the wrong context.

Overall, the provided code patch seems to have areas for improvement, such as addressing code duplication, error handling, and ensuring proper variable scoping.

Expand Down