Skip to content

Conversation

@fanquake
Copy link
Member

This has been the case since #20413.

This should also enable the check for MSVC. From my reading of https://docs.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-160 and https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ if we set the /Zc:__cplusplus switch in additional options, MSVC will report the correct value for __cplusplus. However I have not tested this.

This has already been the case since bitcoin#20413.
From my reading of
https://docs.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-160
and
https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/
if we set the `/Zc:__cplusplus` switch in additional options, MSVC will
report the correct value for `__cplusplus`.
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 5e531e6, checked the MS docs, and AppVeyor build is green.

@laanwj
Copy link
Member

laanwj commented Feb 23, 2021

Code review ACK 5e531e6
(201703L is the correct value for C++17)

@sipa
Copy link
Member

sipa commented Feb 23, 2021

@PS113 Stop spamming this repository.

Copy link
Contributor

@practicalswift practicalswift left a comment

Choose a reason for hiding this comment

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

ACK 5e531e6

Thanks for fixing this: being explicit about our assumptions is a precondition for formal reasoning :)

@maflcko maflcko merged commit 84f6c69 into bitcoin:master Feb 23, 2021
@fanquake fanquake deleted the assuming_c++17 branch February 23, 2021 13:25
@bitcoin bitcoin locked and limited conversation to collaborators Feb 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants