Changes to compile with C++20 #1505
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several changes across multiple files to support the C++20 standard. Key modifications include updating the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
src/validation.cpp (2)
6-6: Clarify the temporary inclusion ofboost_function_epilogue.hppThe inclusion of
boost_function_epilogue.hppis marked as temporary with aTODOcomment. Consider providing additional context or a reference to an issue number to facilitate future maintenance and removal.
Line range hint
459-461: RefactorCheckTransactionto reduce parameter complexityThe
CheckTransactionfunction now has a large number of parameters, which can reduce readability and maintainability. Consider encapsulating related parameters into a struct or class to simplify the function signature.For example, you could define a struct to hold related validation parameters:
struct TransactionValidationOptions { bool fCheckDuplicateInputs; uint256 hashTx; bool isVerifyDB; int nHeight; bool isCheckWallet; bool fStatefulZerocoinCheck; sigma::CSigmaTxInfo *sigmaTxInfo; lelantus::CLelantusTxInfo *lelantusTxInfo; spark::CSparkTxInfo *sparkTxInfo; }; bool CheckTransaction(const CTransaction &tx, CValidationState &state, const TransactionValidationOptions &options)src/qt/pcodemodel.cpp (1)
5-6: Track temporary include dependencyThe TODO comment indicates this include is temporary. Consider creating a tracking issue to remove it after the Boost upgrade.
Would you like me to create a GitHub issue to track the removal of this temporary include?
src/qt/splashscreen.cpp (1)
Remove commented-out code as it's obsolete
The commented-out code represents an old constructor implementation that has been replaced with a new one. This is evident from:
- The header file shows only one active constructor
SplashScreen(const QPixmap &pixmap, Qt::WindowFlags f)while the old constructor is commented out- The new implementation inherits from
QSplashScreeninstead ofQWidgetas noted in the header's documentation- The old implementation is completely commented out and the new one is active and being used
The large block of commented code should be removed as it's no longer needed and adds unnecessary noise to the codebase. This will improve code readability and maintainability.
🔗 Analysis chain
Line range hint
66-256: Consider removing or documenting commented codeLarge sections of commented-out code should either be removed if no longer needed or documented if retained for future reference.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this commented code exists in previous versions git log -S 'SplashScreen::SplashScreen(Qt::WindowFlags f, const NetworkStyle *networkStyle)' src/qt/splashscreen.cppLength of output: 0
Script:
#!/bin/bash # Let's try a different approach to check the history of this file rg -U "class SplashScreen" src/qt/splashscreen.cpp # Also check if there are any other commented constructor implementations rg -U "SplashScreen::" src/qt/splashscreen.cpp # Check if there are any related files that might contain the implementation fd splashscreenLength of output: 876
Script:
#!/bin/bash # Let's check the header file to understand the current class structure cat src/qt/splashscreen.h # Also check if the active constructor implementation exists elsewhere in the file rg -U "SplashScreen::SplashScreen\(const QPixmap" src/qt/splashscreen.cpp -A 10Length of output: 2123
src/qt/automintmodel.cpp (1)
1-1: Track TODO for removing temporary includeThe TODO comment indicates this include is temporary and should be removed after a Boost upgrade.
Would you like me to create a GitHub issue to track the removal of this temporary include after the Boost upgrade is complete?
build-aux/m4/ax_cxx_compile_stdcxx.m4 (1)
963-1015: Comprehensive C++20 Feature TestingThe test body includes checks for key C++20 features:
- Concepts (std::is_integral)
- Ranges (std::ranges::sort)
- Three-way comparison operator (<=>)
Consider adding tests for other C++20 features:
- Coroutines
- Modules (if supported by build system)
- Constexpr virtual functions
Would you like me to provide additional test cases for these features?
src/qt/addresstablemodel.cpp (2)
5-6: Document the Boost compatibility issueThe TODO comment indicates this is a temporary solution. Please add more context about:
- The specific Boost compatibility issue being addressed
- The minimum Boost version that would resolve this issue
- The planned timeline for the Boost upgrade
Line range hint
1-1000: Consider C++20 modernization opportunitiesNow that the codebase is moving to C++20, consider these improvements:
- Replace Boost.Optional with std::optional throughout
- Use C++20 ranges for container operations
- Consider using std::span for array views
- Utilize C++20 concepts for template constraints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (22)
build-aux/m4/ax_cxx_compile_stdcxx.m4(3 hunks)configure.ac(1 hunks)depends/packages/boost.mk(1 hunks)depends/packages/qt.mk(1 hunks)depends/packages/zeromq.mk(1 hunks)src/boost_function_epilogue.hpp(1 hunks)src/qt/addresstablemodel.cpp(1 hunks)src/qt/automintmodel.cpp(2 hunks)src/qt/bitcoin.cpp(1 hunks)src/qt/bitcoingui.cpp(3 hunks)src/qt/clientmodel.cpp(1 hunks)src/qt/pcodemodel.cpp(1 hunks)src/qt/sendtopcodedialog.cpp(1 hunks)src/qt/splashscreen.cpp(1 hunks)src/qt/transactiontablemodel.cpp(1 hunks)src/qt/walletmodel.cpp(2 hunks)src/support/allocators/mt_pooled_secure.h(2 hunks)src/support/allocators/pooled_secure.h(1 hunks)src/support/allocators/secure.h(1 hunks)src/support/allocators/zeroafterfree.h(1 hunks)src/validation.cpp(1 hunks)src/validationinterface.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- src/qt/clientmodel.cpp
- src/validationinterface.cpp
- src/qt/walletmodel.cpp
- src/qt/transactiontablemodel.cpp
- src/qt/bitcoin.cpp
- src/qt/sendtopcodedialog.cpp
- src/qt/bitcoingui.cpp
🔇 Additional comments (19)
src/validation.cpp (1)
Line range hint 76-77: Verify compatibility of AbortNode function overloads with default parameters
The addition of default values to the userMessage parameter in both AbortNode function overloads may introduce ambiguities or affect existing calls. Ensure that all invocations are compatible with the updated signatures to prevent unintended behavior.
Run the following script to identify potential issues:
✅ Verification successful
Let me analyze the results and generate a final response.
No compatibility issues found with AbortNode function overloads
The verification shows that all calls to AbortNode are compatible with the updated signatures:
- Most calls use the two-parameter version with
CValidationStateand provide only thestrMessageparameter, which works correctly with the defaultuserMessageparameter - The direct calls without
CValidationStatealso provide either one or two parameters, matching the function signatures - No ambiguous calls were found that could cause confusion between the two overloads
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all calls to `AbortNode` and check for compatibility with the new default parameter.
# Find all calls to `AbortNode` in the codebase
rg 'AbortNode\('
# Expected Result: All calls should be compatible with the updated function signatures without causing overload ambiguities.
Length of output: 2638
src/boost_function_epilogue.hpp (1)
1-45: Ensure compliance with Boost licensing and coding standards
The new file boost_function_epilogue.hpp addresses a compatibility issue with C++20 and Boost.Function. Verify that the implementation complies with the Boost Software License and follows the project's coding conventions.
depends/packages/zeromq.mk (1)
15-15: Verify ZeroMQ compatibility with C++20
The $(package)_cxxflags have been updated to -std=c++20. Ensure that ZeroMQ version 4.3.1 is fully compatible with C++20 features and that this change does not introduce any compilation or runtime issues.
You can check the official ZeroMQ documentation or release notes for C++20 support.
src/support/allocators/zeroafterfree.h (1)
18-32: Modernize allocator using C++20 features
The updates to zero_after_free_allocator leverage std::allocator_traits and replace throw() with noexcept, aligning with modern C++ practices. Ensure that these changes maintain the intended functionality and are compatible with the C++20 standard.
src/support/allocators/secure.h (2)
23-23: LGTM! Modern allocator traits implementation
The changes properly modernize the allocator implementation by:
- Using std::allocator_traits as recommended by the standard
- Updating type definitions to use modern C++ syntax
- Following C++20 allocator requirements
Also applies to: 26-27, 28-29, 30-30
31-32: LGTM! Exception specification modernization
Correctly updates exception specifications from deprecated throw() to modern noexcept, which is the preferred way in C++20 to indicate that functions won't throw exceptions.
Also applies to: 34-34, 37-37
depends/packages/boost.mk (1)
26-26: Verify C++20 compatibility across all target platforms
While Boost 1.81.0 supports C++20, we should ensure compatibility across all target platforms and toolchains.
✅ Verification successful
Based on the search results, I can make a determination about C++20 compatibility:
- The codebase shows that both Boost and ZeroMQ packages are configured to use C++20 (
-std=c++20flag) - The build documentation indicates minimum compiler requirements that should support C++20:
- Ubuntu 18.04+ is required for building
- Modern GCC/Clang compilers are used across platforms
- Other key dependencies like Berkeley DB are using older C++ standards (C++11) which is compatible with C++20
C++20 compatibility is appropriate for the codebase
The upgrade to C++20 in Boost 1.81.0 is well-aligned with the project's compiler requirements and build environment across supported platforms. The codebase already uses modern C++ features and has proper compiler version requirements documented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check C++20 support in dependencies and toolchains
# Check if configure scripts mention C++20
rg -l "std=(c\+\+|c\+\+20|cpp20)" configure*
# Look for potential C++20 compatibility flags or issues
rg -i "c\+\+20|cpp20|compiler|std=" depends/packages/*.mk
# Search for compiler version requirements
rg -i "compiler|gcc|clang|minimum|require" README* INSTALL* doc/*
Length of output: 33165
src/support/allocators/pooled_secure.h (3)
24-24: LGTM! Consistent modernization with secure.h
The changes maintain consistency with secure.h and properly implement modern allocator requirements:
- Using std::allocator_traits
- Modern type alias syntax
- Following C++20 allocator requirements
Also applies to: 27-28, 29-30, 31-31
38-38: LGTM! Improved type safety with nullptr
Good improvement replacing the literal 0 with nullptr for the hint parameter, which provides better type safety and clarity.
34-36: LGTM! Constructor exception specification
Properly updates the constructor's exception specification to noexcept, maintaining consistency with other methods.
src/support/allocators/mt_pooled_secure.h (3)
21-28: LGTM: Modern type definitions using allocator_traits
The changes properly leverage std::allocator_traits for type definitions, which is the recommended approach in modern C++. This improves maintainability and standardization.
29-31: Update exception specification to modern C++
The constructor's exception specification has been correctly updated from throw() to noexcept, aligning with modern C++ practices.
44-44: Improve type safety with nullptr
Good update to use nullptr instead of 0 for the hint parameter, which provides better type safety and clarity.
src/qt/pcodemodel.cpp (1)
Line range hint 191-219: LGTM: Improved sorting implementation
The sort implementation using lambda functions is well-structured and provides clear comparison logic for each column type.
src/qt/splashscreen.cpp (2)
9-10: Track temporary include dependency
Similar TODO comment about temporary include. This should be addressed in the same tracking issue as the other files.
Line range hint 43-49: LGTM: Proper testnet image handling
The conditional loading of splash screen images based on testnet flag is well-implemented.
src/qt/automintmodel.cpp (1)
Line range hint 1-537: LGTM - C++20 Compatible
The code uses modern C++ features and should compile correctly with C++20. No compatibility issues found.
depends/packages/qt.mk (1)
44-44: Verify compiler support for -c++std c++2a
The change from c++17 to c++2a is correct for C++20 support. However, we should verify compiler compatibility across all target platforms.
✅ Verification successful
Based on the build system configuration and compiler settings found in the codebase, I can generate the final response:
The -c++std c++2a flag change is appropriate and verified
The codebase explicitly requires C++20 support as shown in configure.ac:
dnl Require C++20 compiler (no GNU extensions)
AX_CXX_COMPILE_STDCXX([20], [noext], [mandatory], [nodefault])
The change from c++17 to c++2a in qt.mk is correct because:
- The project mandates C++20 support across all builds
- c++2a was the flag used for C++20 in older compiler versions before C++20 was officially released
- The build system is set up to handle compiler compatibility checks and will fail if C++20 is not supported
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the configured compiler supports c++2a/c++20
# Note: This only checks the compiler version, actual compilation testing will be done separately
# Check compiler version and C++20 support
$CXX --version
$CXX -std=c++2a -dM -E - < /dev/null | grep -i "cplusplus"
Length of output: 254
Script:
#!/bin/bash
# Let's check the build system configuration and compiler settings
# Check build system files for compiler configuration
fd -e mk -e sh . depends/builders | xargs cat
# Check configure scripts for compiler settings
fd configure -e ac -e in . | xargs cat | grep -A 5 -B 5 "c++\|CXX"
# Look for any documentation about build requirements
cat README.md doc/build*.md 2>/dev/null | grep -A 5 -B 5 "compiler\|GCC\|Clang"
Length of output: 18227
build-aux/m4/ax_cxx_compile_stdcxx.m4 (1)
53-53: LGTM - C++20 Alternative Added
The addition of "20 2a" to ax_cxx_compile_alternatives correctly supports both -std=c++20 and -std=c++2a compiler flags.
| dnl Require C++20 compiler (no GNU extensions) | ||
| AX_CXX_COMPILE_STDCXX([20], [noext], [mandatory], [nodefault]) |
There was a problem hiding this comment.
💡 Codebase verification
Update C++ standard settings in dependencies for C++20 compatibility
The verification revealed compatibility concerns that need to be addressed:
- Boost and ZeroMQ are already configured for C++20
- Qt uses c++2a flag (equivalent to C++20)
- BDB is still using an outdated C++11 standard flag
• depends/packages/bdb.mk: Update -std=c++11 to -std=c++20 to maintain consistency with the project's C++20 requirement
🔗 Analysis chain
Verify C++20 compatibility across dependencies
The change to C++20 is aligned with the PR objective. However, we should ensure all dependencies support C++20 compilation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check C++ standard requirements in dependency build files
# Check C++ standard in dependency build files
echo "Checking C++ standard requirements in dependency files..."
rg -l "std=(c\+\+|gnu\+\+)(17|2a|20)" depends/
Length of output: 226
Script:
#!/bin/bash
# Let's check the specific C++ standard settings in these files
echo "=== Checking boost.mk C++ standard settings ==="
rg "std=(c\+\+|gnu\+\+)(17|2a|20)" -A 2 depends/packages/boost.mk
echo -e "\n=== Checking zeromq.mk C++ standard settings ==="
rg "std=(c\+\+|gnu\+\+)(17|2a|20)" -A 2 depends/packages/zeromq.mk
# Let's also check if there are any other C++ dependencies that might need attention
echo -e "\n=== Listing all C++ related package files ==="
fd -e mk . depends/packages/ -x grep -l "c++" {}
Length of output: 783
Script:
#!/bin/bash
# Let's check the remaining C++ dependencies for their standard settings
echo "=== Checking qt.mk C++ standard settings ==="
rg "std=(c\+\+|gnu\+\+)" -A 2 depends/packages/qt.mk
echo -e "\n=== Checking native_clang.mk C++ standard settings ==="
rg "std=(c\+\+|gnu\+\+)" -A 2 depends/packages/native_clang.mk
echo -e "\n=== Checking bdb.mk C++ standard settings ==="
rg "std=(c\+\+|gnu\+\+)" -A 2 depends/packages/bdb.mk
# Let's also check if there are any explicit C++ version requirements in these files
echo -e "\n=== Checking for version requirements or compatibility notes ==="
rg -i "c\+\+\s*(17|20|2a)" depends/packages/{qt,native_clang,bdb}.mk
Length of output: 916
PR intention
The codebase now compiles with C++20
Code changes brief
Requires extensive testing before it can ever end up in master, which will be taken care of in scope of the spats project testing itself.