Use llvm project #361
Merged
Use llvm project #361
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Unit test report for commit a3c98ca. All test cases passed! Successful Test Cases
|
|
Integration test report for commit a3c98ca.
Successful Test Cases
|
|
Unit test report for commit 1ffbd66. All test cases passed! Successful Test Cases
|
|
Integration test report for commit 1ffbd66.
Successful Test Cases
|
|
Unit test report for commit 38be9e4. All test cases passed! Successful Test Cases
|
|
Integration test report for commit 38be9e4.
Successful Test Cases
|
JackyWoo
reviewed
Sep 25, 2024
| /// keep consistent with Coordination::read method by changing big endian to little endian. | ||
| return __builtin_bswap32(res); |
There was a problem hiding this comment.
why not use __builtin_bswap32
There was a problem hiding this comment.
std::byteswap is standardized in C++23, it better than use __builtin_bswap32 which is a GCC/Clang built-in function.
There was a problem hiding this comment.
Please find and replace __builtin_bswap32 in global
|
I will open another pr to fix failed test #363 |
JackyWoo
reviewed
Sep 25, 2024
| @@ -13,6 +13,11 @@ namespace | |||
| template <typename T, typename... Ts> constexpr auto firstArg(T && x, Ts &&...) { return std::forward<T>(x); } | |||
| } | |||
|
|
|||
| template<typename... Args> | |||
There was a problem hiding this comment.
It seems logFormat is meaningless.
src/Service/KeeperStore.cpp
Outdated
| @@ -1018,7 +1019,7 @@ struct StoreRequestMultiTxn final : public StoreRequest | |||
| } | |||
| else | |||
| throw RK::Exception( | |||
| ErrorCodes::BAD_ARGUMENTS, "Illegal command as part of multi ZooKeeper request {}", sub_zk_request->getOpNum()); | |||
| ErrorCodes::BAD_ARGUMENTS, "Illegal command as part of multi ZooKeeper request {}", fmt::underlying(sub_zk_request->getOpNum())); | |||
There was a problem hiding this comment.
Please use toString(sub_zk_request->getOpNum())
src/Service/KeeperStore.cpp
Outdated
| @@ -1153,7 +1154,7 @@ class StoreRequestFactory final : private boost::noncopyable | |||
| void registerRequest(Coordination::OpNum op_num, Creator creator) | |||
| { | |||
| if (!op_num_to_request.try_emplace(op_num, creator).second) | |||
| throw RK::Exception(ErrorCodes::LOGICAL_ERROR, "Request with op num {} already registered", op_num); | |||
| throw RK::Exception(ErrorCodes::LOGICAL_ERROR, "Request with op num {} already registered", fmt::underlying(op_num)); | |||
There was a problem hiding this comment.
Same with the previous one
| @@ -897,7 +897,7 @@ size_t KeeperSnapshotManager::removeSnapshots() | |||
| { | |||
| Int64 remove_count = static_cast<Int64>(snapshots.size()) - static_cast<Int64>(keep_max_snapshot_count); | |||
|
|
|||
| LOG_INFO(log, "There are {} snapshots, we will try to move {remove_count} of them", snapshots.size(), remove_count); | |||
There was a problem hiding this comment.
This is truly a trivial mistake
src/Service/NuRaftStateMachine.cpp
Outdated
| @@ -408,7 +408,7 @@ void NuRaftStateMachine::replayLogs(ptr<log_store> log_store_, uint64_t from, ui | |||
| { | |||
| if (entry_with_version.entry->get_val_type() != nuraft::log_val_type::app_log) | |||
| { | |||
| LOG_DEBUG(thread_log, "Found non app nuraft log(type {}), ignore it", entry_with_version.entry->get_val_type()); | |||
| LOG_DEBUG(thread_log, "Found non app nuraft log(type {}), ignore it", fmt::underlying(entry_with_version.entry->get_val_type())); | |||
There was a problem hiding this comment.
It is better to add a toString method
ok |
|
Do not forget to update the min clang version in readme and how-to-build docs. And throw an error when the version is not meet in CMakelist file. |
|
Unit test report for commit b0b7df9. All test cases passed! Successful Test Cases
|
|
Integration test report for commit b0b7df9. All test cases passed! Successful Test Cases
|
|
Unit test report for commit af425d7. All test cases passed! Successful Test Cases
|
|
Integration test report for commit af425d7. All test cases passed! Successful Test Cases
|
|
Unit test report for commit 430be92. All test cases passed! Successful Test Cases
|
|
Integration test report for commit 430be92.
Successful Test Cases
|
|
Unit test report for commit 5647844. All test cases passed! Successful Test Cases
|
|
Integration test report for commit 5647844. All test cases passed! Successful Test Cases
|
JackyWoo
approved these changes
Sep 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issues of this PR fixes:
This PR is part of #291
Replace the ClickHouse‘s
libcxxandlibcxxabiwith llvm-project.Can't Replace ClickHouse‘s
libunwindnow, because jemalloc still needunw_backtrace, and our stackTrace also useunw_backtraceChange log:
Add llvm-project 17 to instead of ClickHouse‘s
libcxxandlibcxxabi