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

Upgrade from c++17 to c++20 #238

Merged
merged 1 commit into from
Feb 29, 2024
Merged

Upgrade from c++17 to c++20 #238

merged 1 commit into from
Feb 29, 2024

Conversation

AlexRamRam
Copy link
Contributor

Fix #237

Signed-off-by: Alexander Jung <[email protected]>
@maurermi
Copy link
Collaborator

maurermi commented Oct 9, 2023

concept-ACK, I think this is a good idea -- will test soon

@HalosGhost
Copy link
Collaborator

Concept ACK (we had initially avoided 20 till Bitcoin switches, but given we now have dependencies which will require switching, I think that ship has sailed); reminder we need to test basic performance impact locally and in test controller.

Copy link
Collaborator

@maurermi maurermi left a comment

Choose a reason for hiding this comment

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

T-ACK. Tested using Docker and all tests pass. Did not explicitly evaluate performance impact at this time relative to the C++17 build but changes are minimal and it would be quite surprising if this impacted performance as far as I can tell.

The question remains of when exactly we want to make this switch. Sooner seems better than later since later developments may depend on C++20.

Edit: Appended this commit to trunk for the test, can confirm that this change works with current state of trunk.

@HalosGhost
Copy link
Collaborator

tACK. I've checked rebased against the latest trunk, and done some local performance comparisons (no performance degradation is visible; if anything, I see slightly higher sustained throughput for identical configurations).

LGTM. Merging.

@HalosGhost HalosGhost merged commit 9715ff5 into mit-dci:trunk Feb 29, 2024
6 checks passed
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.

Upgrade build to c++20
3 participants