-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix: gcc13 compile failed #1601
fix: gcc13 compile failed #1601
Conversation
@@ -7,7 +7,7 @@ | |||
#define PIKA_DATA_DISTRIBUTION_H_ | |||
|
|||
#include <cstdint> | |||
#include "pstd/include/pstd_status.h" | |||
#include <string> | |||
|
|||
// polynomial reserved Crc32 magic num | |||
const uint32_t IEEE_POLY = 0xedb88320; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the code patch looks relatively simple and straightforward since it involves only adding the <string>
header and updating a comment.
One potential concern is that changing the header inclusion may cause unintended side effects, depending on how other parts of the codebase are using the pstd_status.h
header. It would be a good idea to test the modified code carefully and ensure that it still builds and functions correctly.
As for improvement suggestions, it's difficult to say without more context about the purpose and structure of the code. However, one general tip for code review is to strive for consistency in coding style and naming conventions throughout the codebase. Another suggestion is to use descriptive variable names that are easy to understand and maintain over time.
@@ -5,6 +5,8 @@ | |||
|
|||
#include "include/pika_data_distribution.h" | |||
|
|||
#include <cassert> | |||
|
|||
const std::string kTagBegin = "{"; | |||
const std::string kTagEnd = "}"; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code patch is very small and only adds an include statement for the C++ assert function. It also defines two string constants.
Since there are no changes to any existing functionality, there are no obvious bug risks introduced by this patch. However, it's difficult to provide improvement suggestions without more context or information about the purpose of this code.
Regarding the use of assert, it's generally good practice to use it to check assumptions that you believe to always be true at runtime. If the assumption is ever violated, assert will immediately terminate the program and print an error message, making it easier to identify and fix bugs. Just make sure not to use assert for input validation or checking user-provided data, as assertions can be disabled in release builds, meaning they won't catch errors in the deployed software.
src/pstd/src/pstd_status.cc
Outdated
@@ -5,6 +5,7 @@ | |||
#include "pstd/include/pstd_status.h" | |||
#include <cstdint> | |||
#include <cstdio> | |||
#include <cstdint> | |||
|
|||
namespace pstd { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code appears to simply include some headers and define a namespace, so there isn't much to review. However, the cstdint
header is included twice, which could potentially cause issues if there are conflicting declarations. It would be better to remove the duplicated inclusion of cstdint
. Otherwise, without more context or information about what this code does, it is difficult to provide any further suggestions for improvement or bug risk assessment.
@@ -40,6 +40,7 @@ | |||
#include <climits> | |||
#include <cmath> | |||
#include <cstdio> | |||
#include <cstdint> | |||
#include <cstdlib> | |||
#include <cstring> | |||
#include <sstream> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code patch adds the <cstdint>
header which contains typedefs for exact-width integer types such as int8_t
, uint16_t
, and others. This is a good practice as it ensures that the code uses fixed-size integer types regardless of the platform or compiler being used.
Without seeing the rest of the code, it is difficult to identify any specific bug risks or improvements. However, it is generally a good practice to use descriptive variable names, avoid using global variables, and properly manage memory allocations and deallocations to avoid memory leaks. Additionally, consider using more modern C++ features such as smart pointers, range-based loops, and lambda expressions to improve readability and maintainability.
Overall, adding the <cstdint>
header seems like a beneficial change, but further review of the code is necessary to identify potential issues and make additional improvements.
@@ -7,7 +7,7 @@ | |||
#define PIKA_DATA_DISTRIBUTION_H_ | |||
|
|||
#include <cstdint> | |||
#include "pstd/include/pstd_status.h" | |||
#include <string> | |||
|
|||
// polynomial reserved Crc32 magic num | |||
const uint32_t IEEE_POLY = 0xedb88320; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a brief code review:
-
The code patch appears to be a small modification that replaces the header file
pstd_status.h
with<string>
. -
There is no obvious bug risk in this patch, as it does not change any functional behavior in the code.
-
One potential improvement suggestion could be to use standard library headers wherever possible instead of custom libraries or headers. This can help ensure better portability and maintenance of the code.
-
Additionally, adding comments to the code can make it more understandable for other developers working on the same project.
-
Finally, it's always good practice to run existing tests or add new tests to ensure the modified code patch doesn't introduce unexpected issues.
@@ -5,6 +5,8 @@ | |||
|
|||
#include "include/pika_data_distribution.h" | |||
|
|||
#include <cassert> | |||
|
|||
const std::string kTagBegin = "{"; | |||
const std::string kTagEnd = "}"; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code patch seems to simply add an #include
directive and a const
definition for two string variables.
As for the added #include <cassert>
line, it indicates that the code will now make use of the assert()
function in C++. This can be helpful for checking assumptions made during debugging or development, but it is not strictly necessary.
Regarding the const std::string
definitions for kTagBegin
and kTagEnd
, the naming convention is clear and they appear to be properly initialized. However, depending on their usage elsewhere in the code, it may be worth considering making them constexpr
instead of just const
.
Overall, this patch seems relatively small and safe, with no apparent bug risks or major improvements needed.
@@ -3,7 +3,7 @@ | |||
// found in the LICENSE file. See the AUTHORS file for names of contributors. | |||
|
|||
#include "pstd/include/pstd_status.h" | |||
#include <cstdint> | |||
#include <cstdint> | |||
#include <cstdio> | |||
|
|||
namespace pstd { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provided code patch appears to be a small change where a whitespace was added after the include statement for "cstdint". This change is unlikely to introduce any bug risk or have any negative impact on the code. However, since this is a very small change, it's difficult to provide any improvement suggestions based solely on the code patch.
* feat: supported 'ptype' command * Simplified command `type` and `ptype` code * Remove the capacity judgment * fix slavaof serialize response bug (#1583) Co-authored-by: liuyuecai <[email protected]> * fix (#1587) (#1588) * fix unit test:type/set (#1577) * Fix memory leaks in HandleBGWorkerWriteDB and HandleMetaSyncResponse (#1590) * fix memory leaks in PikaReplBgWorker::HandleBGWorkerWriteDB(void* arg) and PikaReplClientConn::HandleMetaSyncResponse(void* arg). * add address/thread sanitizer to CMakeLists --------- Co-authored-by: cjh <[email protected]> * refactor: replace pstd/env with std::filesystem (#1470) * fix bug issue 1554: using unique_ptr.reset to fix the SIGABRT error (#1595) using unique_ptr.reset to fix the SIGABRT error && using time-wait lock to notify quickly * fix: incorrect manner of terminating the process (#1596) * fix issue#1597: add rocksdb dependency to pstd (#1599) * fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]> * fix_info_command * fix: gcc13 compile failed (#1601) * ci: add unit test to github action (#1604) * ci: add unit test in github workflow * chore:change `PLATFORM` field logic (#1615) * fix issue 1517: scan 命令不支持 type 参数 (#1582) * fix: the unit test of type/set (#1617) * test: optimize the return results of srandmember to avoid approximate results * fix: use last_seed for random engine * [fix issue1621] fix deadlock (#1620) * [fix] fix deadlock * [fix] fix * command `type` and `ptype` add unit test * fix member variable initialization * Update issue templates * feat: supported 'ptype' command * fix unit test:type/set (#1577) * fix issue#1597: add rocksdb dependency to pstd (#1599) * fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]> * fix: gcc13 compile failed (#1601) * fix: the unit test of type/set (#1617) * test: optimize the return results of srandmember to avoid approximate results * fix: use last_seed for random engine * Modify other modules to use the new `type` function * fix function repeat --------- Co-authored-by: Yuecai Liu <[email protected]> Co-authored-by: liuyuecai <[email protected]> Co-authored-by: Peter Chan <[email protected]> Co-authored-by: chenbt <[email protected]> Co-authored-by: Junhua Chen <[email protected]> Co-authored-by: cjh <[email protected]> Co-authored-by: kang jinci <[email protected]> Co-authored-by: Xin.Zh <[email protected]> Co-authored-by: machinly <[email protected]> Co-authored-by: A2ureStone <[email protected]> Co-authored-by: J1senn <[email protected]> Co-authored-by: chejinge <[email protected]> Co-authored-by: baerwang <[email protected]> Co-authored-by: ptbxzrt <[email protected]> Co-authored-by: 你不要过来啊 <[email protected]>
* feat: supported 'ptype' command * Simplified command `type` and `ptype` code * Remove the capacity judgment * fix slavaof serialize response bug (OpenAtomFoundation#1583) Co-authored-by: liuyuecai <[email protected]> * fix (OpenAtomFoundation#1587) (OpenAtomFoundation#1588) * fix unit test:type/set (OpenAtomFoundation#1577) * Fix memory leaks in HandleBGWorkerWriteDB and HandleMetaSyncResponse (OpenAtomFoundation#1590) * fix memory leaks in PikaReplBgWorker::HandleBGWorkerWriteDB(void* arg) and PikaReplClientConn::HandleMetaSyncResponse(void* arg). * add address/thread sanitizer to CMakeLists --------- Co-authored-by: cjh <[email protected]> * refactor: replace pstd/env with std::filesystem (OpenAtomFoundation#1470) * fix bug issue 1554: using unique_ptr.reset to fix the SIGABRT error (OpenAtomFoundation#1595) using unique_ptr.reset to fix the SIGABRT error && using time-wait lock to notify quickly * fix: incorrect manner of terminating the process (OpenAtomFoundation#1596) * fix issue#1597: add rocksdb dependency to pstd (OpenAtomFoundation#1599) * fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]> * fix_info_command * fix: gcc13 compile failed (OpenAtomFoundation#1601) * ci: add unit test to github action (OpenAtomFoundation#1604) * ci: add unit test in github workflow * chore:change `PLATFORM` field logic (OpenAtomFoundation#1615) * fix issue 1517: scan 命令不支持 type 参数 (OpenAtomFoundation#1582) * fix: the unit test of type/set (OpenAtomFoundation#1617) * test: optimize the return results of srandmember to avoid approximate results * fix: use last_seed for random engine * [fix issue1621] fix deadlock (OpenAtomFoundation#1620) * [fix] fix deadlock * [fix] fix * command `type` and `ptype` add unit test * fix member variable initialization * Update issue templates * feat: supported 'ptype' command * fix unit test:type/set (OpenAtomFoundation#1577) * fix issue#1597: add rocksdb dependency to pstd (OpenAtomFoundation#1599) * fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]> * fix: gcc13 compile failed (OpenAtomFoundation#1601) * fix: the unit test of type/set (OpenAtomFoundation#1617) * test: optimize the return results of srandmember to avoid approximate results * fix: use last_seed for random engine * Modify other modules to use the new `type` function * fix function repeat --------- Co-authored-by: Yuecai Liu <[email protected]> Co-authored-by: liuyuecai <[email protected]> Co-authored-by: Peter Chan <[email protected]> Co-authored-by: chenbt <[email protected]> Co-authored-by: Junhua Chen <[email protected]> Co-authored-by: cjh <[email protected]> Co-authored-by: kang jinci <[email protected]> Co-authored-by: Xin.Zh <[email protected]> Co-authored-by: machinly <[email protected]> Co-authored-by: A2ureStone <[email protected]> Co-authored-by: J1senn <[email protected]> Co-authored-by: chejinge <[email protected]> Co-authored-by: baerwang <[email protected]> Co-authored-by: ptbxzrt <[email protected]> Co-authored-by: 你不要过来啊 <[email protected]>
* feat: supported 'ptype' command * Simplified command `type` and `ptype` code * Remove the capacity judgment * fix slavaof serialize response bug (OpenAtomFoundation#1583) Co-authored-by: liuyuecai <[email protected]> * fix (OpenAtomFoundation#1587) (OpenAtomFoundation#1588) * fix unit test:type/set (OpenAtomFoundation#1577) * Fix memory leaks in HandleBGWorkerWriteDB and HandleMetaSyncResponse (OpenAtomFoundation#1590) * fix memory leaks in PikaReplBgWorker::HandleBGWorkerWriteDB(void* arg) and PikaReplClientConn::HandleMetaSyncResponse(void* arg). * add address/thread sanitizer to CMakeLists --------- Co-authored-by: cjh <[email protected]> * refactor: replace pstd/env with std::filesystem (OpenAtomFoundation#1470) * fix bug issue 1554: using unique_ptr.reset to fix the SIGABRT error (OpenAtomFoundation#1595) using unique_ptr.reset to fix the SIGABRT error && using time-wait lock to notify quickly * fix: incorrect manner of terminating the process (OpenAtomFoundation#1596) * fix issue#1597: add rocksdb dependency to pstd (OpenAtomFoundation#1599) * fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]> * fix_info_command * fix: gcc13 compile failed (OpenAtomFoundation#1601) * ci: add unit test to github action (OpenAtomFoundation#1604) * ci: add unit test in github workflow * chore:change `PLATFORM` field logic (OpenAtomFoundation#1615) * fix issue 1517: scan 命令不支持 type 参数 (OpenAtomFoundation#1582) * fix: the unit test of type/set (OpenAtomFoundation#1617) * test: optimize the return results of srandmember to avoid approximate results * fix: use last_seed for random engine * [fix issue1621] fix deadlock (OpenAtomFoundation#1620) * [fix] fix deadlock * [fix] fix * command `type` and `ptype` add unit test * fix member variable initialization * Update issue templates * feat: supported 'ptype' command * fix unit test:type/set (OpenAtomFoundation#1577) * fix issue#1597: add rocksdb dependency to pstd (OpenAtomFoundation#1599) * fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]> * fix: gcc13 compile failed (OpenAtomFoundation#1601) * fix: the unit test of type/set (OpenAtomFoundation#1617) * test: optimize the return results of srandmember to avoid approximate results * fix: use last_seed for random engine * Modify other modules to use the new `type` function * fix function repeat --------- Co-authored-by: Yuecai Liu <[email protected]> Co-authored-by: liuyuecai <[email protected]> Co-authored-by: Peter Chan <[email protected]> Co-authored-by: chenbt <[email protected]> Co-authored-by: Junhua Chen <[email protected]> Co-authored-by: cjh <[email protected]> Co-authored-by: kang jinci <[email protected]> Co-authored-by: Xin.Zh <[email protected]> Co-authored-by: machinly <[email protected]> Co-authored-by: A2ureStone <[email protected]> Co-authored-by: J1senn <[email protected]> Co-authored-by: chejinge <[email protected]> Co-authored-by: baerwang <[email protected]> Co-authored-by: ptbxzrt <[email protected]> Co-authored-by: 你不要过来啊 <[email protected]>
No description provided.