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

doc: note that you can assume C++20. #30136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rustyrussell
Copy link
Contributor

We check this in configure.ac, line 99:

dnl Require C++20 compiler (no GNU extensions)

This was introduced in:

commit fa67f096bdea9db59dd20c470c9e32f3dac5be94
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date:   Sun Aug 27 10:45:39 2023 +0200

    build: Require C++20 compiler

Which git says was before v27.0rc1:

$ git describe --contains fa67f096bdea
v27.0rc1~224^2~4

We check this in configure.ac, line 99:

	dnl Require C++20 compiler (no GNU extensions)

This was introduced in:

	commit fa67f09
	Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
	Date:   Sun Aug 27 10:45:39 2023 +0200

	    build: Require C++20 compiler

Which git says was before v27.0rc1:

	$ git describe --contains fa67f09
	v27.0rc1~224^2~4

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@DrahtBot
Copy link
Contributor

DrahtBot commented May 18, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK epiccurious

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Docs label May 18, 2024
@@ -771,6 +771,8 @@ Wallet
General C++
-------------

As of v27.0, we require a compiler which meets at least the C++20 standard.
Copy link
Member

@laanwj laanwj May 18, 2024

Choose a reason for hiding this comment

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

It's useful to document what version of the C++ standard can be used, but i don't think this needs to mention the version it was introduced (the document always applies to the branch it is in), nor is this really the place for documenting a compiler requirement (there's dependencies.md for that).

Would word it as "Code needs to adhere to the C++20 standard.".

Copy link
Contributor

Choose a reason for hiding this comment

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

Would word it as "Code needs to adhere to the C++20 standard.".

I second this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, wasn't sure this was the place, but is there somewhere better? Seems like all the regulars "know" this, so putting it where a beginner might look is helpful.

Was thinking the version info is useful for backporting, but that's a limited window and is already in the commit message if people want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding this info here as you propose with the wording suggested by @laanwj would have some chance to be merged (I would ack it).

PS: edited as I realized part of my comment was not adding to the PR itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you're saying. I was prefer "you can assume c++20" (which was my issue, using <bit>, not "you must meet c++20" which implies you understand all nuances of C++ standards?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, i'm fine with wording it in a way that doesn't imply people understand all nuances, say "The code can use the C++20 standard.", but due to the context it should be a statement about the code, not a compiler requirement.

Copy link
Contributor

@ajtowns ajtowns May 26, 2024

Choose a reason for hiding this comment

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

"you can assume c++20"

Note that it depends on compiler support for the individual features; for instance modules are nominally part of C++20, but still have pretty poor compiler support, so aren't something we can make use of. Conversely, sometimes if a feature is widely supported by compilers/libs, we'll use it even if it's not part of the current language standard (eg #24531 #25472).

In that context, not sure if documenting C++20 is all that helpful; if you're unsure, seems better to ask ("Discuss project-specific development on #bitcoin-core-dev on Libera Chat" per doc/README.md). No particular objection either though.

which was my issue, using <bit>,

For <bit> in particular, that feature's been pulled in by #29085 a few months ago. The C++20 PR was #28349 and discussion was in #23363 since they don't seem to have already been mentioned here.

Copy link
Member

@laanwj laanwj May 26, 2024

Choose a reason for hiding this comment

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

Right, in practice not even all C++11 features are fully supported, nor always desirable, e.g. don't thread_local unless it's for a very good reason and only for POD types. But as starting point before listing all the exceptions i think it still makes sense to mention.

E.g. to mention an extreme, if you write code in a purely C++98 way it's likely to get rejected on review.

@epiccurious
Copy link

Concept ACK f00801c.

@Amarrufo-1

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants