【WIP】Fix the clang-tidy checks (little by little) #1656

longfar-ncy · 2023-06-28T09:38:28Z

Fix some of the content according to the suggestions made in the last code review.
And fix the code after rebasing the latest branch.

… with std::to_string().

… is called

cr-gpt · 2023-06-28T09:38:46Z

CMakeLists.txt

+    -extra-arg=-std=c++17
+)


Based on the code patch you provided, here are some observations and suggestions:

In the add_custom_target block, a new -extra-arg option with -std=c++17 is added both to the clang-tidy target and the clang-tidy-fix target. This implies that both targets will use C++17 as a language standard.

It's generally recommended to have consistent settings throughout a project. If your project uses C++17 as the language standard, it would be better to define this setting in a centralized location rather than duplicating it in multiple places. You can set the C++ standard in the project's CMakeLists.txt file using the set(CMAKE_CXX_STANDARD 17) command.

Make sure that the necessary dependencies, such as clang, clang-apply-replacements, and the required C++17 support, are properly installed and available in the build environment.

Other than these observations, the code patch appears to be fine. It adds the -extra-arg=-std=c++17 option to the clang-tidy and clang-tidy-fix targets, which suggests that C++17 syntax is used correctly within the project.

longfar-ncy · 2023-06-28T09:40:08Z

I'm sorry I accidentally deleted the PR before.

…mbers but not content

cr-gpt · 2023-06-28T16:16:21Z

include/pika_command.h

+void RedisAppendLenUint64(std::string& str, uint64_t ori, const std::string& prefix) {
+  RedisAppendLen(str, static_cast<int64_t>(ori), prefix);
+}
+
 void TryAliasChange(std::vector<std::string>* argv);

 #endif


Overall, the code patch seems to involve improvements and bug fixes. Here are some key findings and suggestions:

Header inclusion: The #include <memory> directive is moved up to be included before any other headers that rely on it. This is a good practice to ensure proper dependencies are satisfied.

RedisAppendLenUint64: A new function RedisAppendLenUint64 is introduced as an inline function to append a uint64_t length to a string with a given prefix. This addition helps improve code readability and type safety.

Naming conventions: The function name CheckArg in the Cmd class is modified to accept a uint64_t number instead of an int. This change improves consistency and avoids potential issues when using large numbers of arguments.

Logging: There doesn't seem to be any particular issue with the LogCommand method or its usage. However, without seeing the implementation details, it's challenging to provide specific feedback on its effectiveness or potential areas for improvement.

Code comments: The provided comments sufficiently explain the purpose and recommended usage of certain functions, such as AppendStringLenUint64 and AppendArrayLenUint64. These comments help clarify their intended use, particularly when dealing with large sizes.

Bug risk assessment: Without a full understanding of the codebase and its context, it's difficult to assess potential bugs or risks. Reviewing the entire codebase and considering test cases would be necessary for a comprehensive assessment.

The code patch primarily introduces improvements related to type safety and readability. It's always a good practice to thoroughly test the changes and consider using static analysis tools to identify any potential issues that might not be apparent from a high-level review.

…function with std::to_string()." due to ctest failed This reverts commit 23ebbe8.

cr-gpt · 2023-06-29T08:15:36Z

src/storage/src/util.cc

@@ -227,7 +231,7 @@ int CalculateDataStartAndEndKey(const std::string& key, std::string* data_start_
  dst_ptr += sizeof(int32_t);
  std::strncpy(dst_ptr, key.data(), key.size());
  dst_ptr += key.size();
-  *dst_ptr = static_cast<uint8_t>(0xff);
+  *dst_ptr = static_cast<char>(0xff);

  data_start_key->assign(start, sizeof(int32_t) + key.size());
  data_end_key->assign(start, sizeof(int32_t) + key.size() + 1);


Here are some observations and suggestions for the code patch:

In the function StringMatchUint64, it seems you are trying to handle 64-bit unsigned integers (uint64_t), but the function name and parameter types indicate it should be handling 32-bit signed integers (int). If you intend to handle uint64_t, consider updating the function name, parameter names, and types consistently.

In the function LongDoubleToStr, there are two lines commented out where len is assigned the value of either 3 or 4. If these lines are not needed anymore, you can remove them to avoid confusion.

In the function CalculateMetaStartAndEndKey and CalculateDataStartAndEndKey, casting 0xff to static_cast<char>(0xff) appears unnecessary since 0xff is already within the range of char.

Overall, the code doesn't seem to have major bug risks based on what is shown in the code patch. However, it would be helpful to review the rest of the codebase to ensure the changes made here don't introduce subtle issues. Furthermore, detailed code review would require a deeper understanding of the context and related code.

…g for some header files

cr-gpt · 2023-06-29T14:04:28Z

CMakeLists.txt

+    -extra-arg=-std=c++17
+)


The code patch you provided seems to remove the -header-filter option from both the add_custom_target commands. This option is used to specify a regular expression filter for the files that will be analyzed by clang-tidy. Without the -header-filter, clang-tidy will analyze all the files in the project directory.

If the removal of the -header-filter option was intentional and you want to analyze all files, then there are no bug risks per se. However, keep in mind that analyzing all files can significantly increase the analysis time, especially for large projects.

As for improvement suggestions, here are a few:

Consider keeping the -header-filter option or modify it to filter only specific directories or files that you need to analyze. This can help reduce unnecessary analysis and improve performance.

It's generally a good practice to provide a newline at the end of the file to ensure consistency with coding standards.

Ensure that the paths specified for ${CMAKE_CURRENT_SOURCE_DIR} and ${CMAKE_BINARY_DIR} are correct and point to the desired locations.

Overall, without more context about your project and the intention behind the code changes, it's difficult to provide specific recommendations. It's always a good idea to thoroughly test the modified code to ensure it behaves as expected.

cr-gpt · 2023-06-29T14:04:42Z

include/pika_define.h

@@ -267,7 +267,7 @@ class RmNode : public Node {
  const std::string& DBName() const { return slot_info_.db_name_; }
  uint32_t SlotId() const { return slot_info_.slot_id_; }
  const SlotInfo& NodeSlotInfo() const { return slot_info_; }
-  void SetSessionId(uint32_t session_id) { session_id_ = session_id; }
+  void SetSessionId(int32_t session_id) { session_id_ = session_id; }
  int32_t SessionId() const { return session_id_; }
  std::string ToString() const {
    return "slot=" + DBName() + "_" + std::to_string(SlotId()) + ",ip_port=" + Ip() + ":" +


In the code patch you provided, here are a few observations and suggestions for improvement:

In the definition of PIKA_TTL_NONE and PIKA_TTL_STALE, it is recommended to enclose the values in parentheses for clarity. So instead of #define PIKA_TTL_NONE -1, you can use #define PIKA_TTL_NONE (-1).

In the SetSessionId function of the RmNode class, the parameter type is int32_t, but the setter name suggests that it should be uint32_t instead. If session ids are always positive, consider changing the parameter type to uint32_t as per the getter (SessionId()).

In the ToString() function of the RmNode class, string concatenation using the + operator can be error-prone and less efficient. Consider using std::ostringstream or fmt::format (if available) for better string formatting and concatenation.

Ensure that all necessary classes and headers are included in the implementation file if they are not shown in the provided code snippet.

It would be helpful to review the surrounding context and other parts of the codebase to provide a more comprehensive code review.

Remember to thoroughly test your code after making any changes to ensure proper functionality and identify any potential bugs.

chejinge · 2023-07-01T12:08:03Z

include/pika_conf.h

@@ -27,7 +27,7 @@
 class PikaConf : public pstd::BaseConf {
 public:
  PikaConf(const std::string& path);
-  ~PikaConf() override {}
+  ~PikaConf() override = default;


析构函数也要赋初始值吗，感觉这种没必要吧

clang-tidy 检查项 modernize-use-equals-default 建议使用显式 default 和 delete ，可为编译器优化提供更多可能

AlexStocks · 2023-07-07T12:52:23Z

    * 0617 二期处理一些后续需要做的任务，先列一个issue
    * 0701 提 PR https://github.com/OpenAtomFoundation/pika/pull/1656，快速review合一下
    * 0701 还有 int32、int64、命名问题需要继续做
    * 0805 梳理一下现有没做完的任务，建个issue

longfar added 10 commits June 28, 2023 10:49

fix: clang-tidy checks clang-analyzer-*

46c8002

fix: clang-tidy checks modernize-*

1cdbc23

fix: clang-tidy checks performance-*

c4bfc09

fix: clang-tidy checks bugprone-narrowing-conversions

9318fac

fix: clang-tidy checks clang-analyzer-deadcode.DeadStores

369f11a

fix: fixed some of the content according to the code review suggestions

eb8b598

fix: Replaced the original contents of the LongDoubleToStr() function…

23ebbe8

… with std::to_string().

fix: Change the parameter of StringMatch() (uint64_t -> size_t)

be3125e

fix: Add function StringMatchUint64() and replace where StringMatch()…

e93c6ae

… is called

fix: Fixed some issues that arose after rebase

90ed04a

cr-gpt bot reviewed Jun 28, 2023

View reviewed changes

longfar added 2 commits June 28, 2023 22:54

fix: Fixed a bug where the command 'zrevrangebylex' only displayed nu…

846a126

…mbers but not content

Merge remote-tracking branch 'pika/unstable' into fix/clang-tidy

2cc7d84

cr-gpt bot reviewed Jun 28, 2023

View reviewed changes

Revert "fix: Replaced the original contents of the LongDoubleToStr() …

4ffb1e7

…function with std::to_string()." due to ctest failed This reverts commit 23ebbe8.

cr-gpt bot reviewed Jun 29, 2023

View reviewed changes

fix: improved the previous header filter in CMakeLists.txt not workin…

e6e67b8

…g for some header files

cr-gpt bot reviewed Jun 29, 2023

View reviewed changes

chejinge reviewed Jul 1, 2023

View reviewed changes

luky116 changed the title ~~Fix the clang-tidy checks (little by little)~~ 【WIP】Fix the clang-tidy checks (little by little) Jul 8, 2023

longfar-ncy closed this Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

【WIP】Fix the clang-tidy checks (little by little) #1656

【WIP】Fix the clang-tidy checks (little by little) #1656

longfar-ncy commented Jun 28, 2023

cr-gpt bot Jun 28, 2023

longfar-ncy commented Jun 28, 2023

cr-gpt bot Jun 28, 2023

cr-gpt bot Jun 29, 2023

cr-gpt bot Jun 29, 2023

cr-gpt bot Jun 29, 2023

chejinge Jul 1, 2023

longfar-ncy Jul 1, 2023

AlexStocks commented Jul 7, 2023 •

edited by Mixficsol

Loading

【WIP】Fix the clang-tidy checks (little by little) #1656

【WIP】Fix the clang-tidy checks (little by little) #1656

Conversation

longfar-ncy commented Jun 28, 2023

cr-gpt bot Jun 28, 2023

Choose a reason for hiding this comment

longfar-ncy commented Jun 28, 2023

cr-gpt bot Jun 28, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 29, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 29, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 29, 2023

Choose a reason for hiding this comment

chejinge Jul 1, 2023

Choose a reason for hiding this comment

longfar-ncy Jul 1, 2023

Choose a reason for hiding this comment

AlexStocks commented Jul 7, 2023 • edited by Mixficsol Loading

AlexStocks commented Jul 7, 2023 •

edited by Mixficsol

Loading