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.

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

Resolve compilation and warning issues #1740

Merged
merged 2 commits into from
Jul 17, 2023
Merged

Resolve compilation and warning issues #1740

merged 2 commits into from
Jul 17, 2023

Conversation

Mixficsol
Copy link
Collaborator

@Mixficsol Mixficsol commented Jul 17, 2023

fix #1725

src/pika.cc Outdated
@@ -188,7 +188,7 @@ int main(int argc, char* argv[]) {
PikaSignalSetup();
InitCRC32Table();

LOG(INFO) << "Server at: " << path;
LOG(INFO) << "Servr at: " << path;
g_pika_cmd_table_manager = std::make_unique<PikaCmdTableManager>();
g_pika_server = new PikaServer();
g_pika_rm = std::make_unique<PikaReplicaManager>();
Copy link

Choose a reason for hiding this comment

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

In the code patch you provided, there is only one line change:

- LOG(INFO) << "Server at: " << path;
+ LOG(INFO) << "Servr at: " << path;

It appears that there is a typo in the log message. The word "Server" is spelled as "Servr". If this is unintentional, it should be corrected to "Server" for accuracy.

Other than that, based on the limited code snippet, it is difficult to identify any potential bugs or improvement suggestions. A more comprehensive review would require examining the entire codebase and its context.

@@ -37,7 +37,7 @@ class Version final : public pstd::noncopyable {

void debug() {
std::shared_lock l(rwlock_);
printf("Current pro_num %u pro_offset %lu\n", pro_num_, pro_offset_);
printf("Current pro_num %u pro_offset %llu\n", pro_num_, pro_offset_);
}

private:
Copy link

Choose a reason for hiding this comment

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

Based on the code patch you provided, here's a brief code review:

  1. The code change in the debug() function replaces %lu formatting with %llu formatting for printing pro_offset_. This change suggests that pro_offset_ is of type unsigned long long.

    Bug Risk: If pro_offset_ is not actually an unsigned long long or its value exceeds the range of unsigned long long, there could be undefined behavior or incorrect output. Make sure the data types used in the printf statement match the actual types of the variables being printed. Consider using type-safe input/output functions instead, such as std::cout for formatted output.

  2. It seems that the class Version extends pstd::noncopyable. Without further information about pstd::noncopyable, it's difficult to comment on its correctness or potential improvements.

Improvement suggestions:

  • Prefer using C++ stream-based input/output (std::cout) rather than C-style printf for better type safety and readability.
  • Consider adding comments to clarify the purpose and behavior of the class and its member functions.
  • Review other parts of the codebase to ensure consistency in data types used for similar values and their corresponding format specifiers in printf or equivalent statements.

Please note that this review is based only on the code snippet you provided, and a complete code analysis may require examining the entire codebase and considering various factors specific to your project requirements.

@@ -371,7 +371,7 @@ static void GetAllNeighbors(const std::shared_ptr<Slot>& slot, std::string& key,
double distance = geohashGetDistance(longitude, latitude, xy[0], xy[1]);
distance = length_converter(distance, range.unit);
char buf[32];
sprintf(buf, "%.4f", distance);
snprintf(buf, sizeof(buf), "%.4f", distance);
res.AppendStringLenUint64(strlen(buf));
res.AppendContent(buf);
}
Copy link

Choose a reason for hiding this comment

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

The code patch you provided seems to address two main areas: formatting a double value into a string using sprintf and using snprintf instead, and iterating over a collection of neighbors and appending their distances to a result.

Regarding the changes made:

  1. In the first code block, the sprintf function is replaced with snprintf. This is generally considered safer because snprintf ensures that the output won't exceed the buffer size specified as an argument, preventing potential buffer overflow issues.

  2. Similarly, in the second code block, the sprintf call is replaced with snprintf, following the same reasoning for increased safety.

These changes effectively mitigate the risk of buffer overflows caused by unsafe use of the sprintf function.

Improvement suggestions:

  1. Consider using a larger buffer size than 32 for the buf array to accommodate larger numeric values if necessary. Insufficient buffer size can still lead to truncation or unexpected behavior even with snprintf.

  2. Verify that the length converter (length_converter) handles all possible unit conversions correctly and consistently.

  3. Ensure that the geohashGetDistance function handles edge cases or invalid inputs gracefully to avoid potential errors or exceptions.

  4. Consider validating the input parameters (first_xy, second_xy, longitude, latitude, etc.) before passing them to functions. Validate ranges, data types, or potential null/empty values to prevent possible issues.

  5. Add comments or documentation within the code to provide better understanding and context for future maintainers.

Remember to thoroughly test your code after making any changes to ensure correctness and proper functionality.

@Mixficsol Mixficsol changed the title 【WIP】just test 【WIP】Resolve compilation and warning issues Jul 17, 2023
Copy link
Contributor

@yaoyinnan yaoyinnan left a comment

Choose a reason for hiding this comment

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

LGTM

@yaoyinnan yaoyinnan changed the title 【WIP】Resolve compilation and warning issues Resolve compilation and warning issues Jul 17, 2023
@chejinge chejinge merged commit bac8102 into OpenAtomFoundation:unstable Jul 17, 2023
8 checks passed
@Mixficsol Mixficsol deleted the atom_unstable branch July 17, 2023 05:00
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
@coderabbitai coderabbitai bot mentioned this pull request Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

解决pika编译时的warning
3 participants