Request C++17 by default, add fallback flag for now #3063
Conversation
|
Should we put this in the 5.0 branch? |
|
In my opinion no - the real breaking change would actually be when we remove the flag and start using C++17-only constructs in the code. |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
make/compiler_flags
Outdated
| ifeq ($(STAN_USE_CPP14),) | ||
| CXXFLAGS_LANG ?= -std=c++17 | ||
| else | ||
| $(warning "Because STAN_USE_CPP14 is set C++14 standard is requested. This will not be possible in the next release!") | ||
| CXXFLAGS_LANG ?= -std=c++1y | ||
| endif |
There was a problem hiding this comment.
I don't like that this is going to cause compiler errors for users who do not have c++17. Can we do something like the below? Idt this is very portable, but if we can find a way to pull the value out from windows then we can avoid having user code error out
CPP_STANDARD := $(shell $(CXX) -x c++ -E -dM - < /dev/null | grep -oP '__cplusplus \K\d+')
# Convert the CPP_STANDARD into a format usable by Make's conditionals
IS_CPP_STANDARD_GREATER_THAN_2017 := $(shell [ $(CPP_STANDARD) -gt 201700 ] && echo 1 || echo 0)
# Conditional directive based on the CPP_STANDARD
ifeq ($(IS_CPP_STANDARD_GREATER_THAN_2017),1)
CXXFLAGS_LANG ?= -std=c++17
else
$(warning "Your compiler does not support C++17 which will be required in the next release! Please upgrade your compiler.")
CXXFLAGS_LANG ?= -std=c++1y
endifThere was a problem hiding this comment.
That won't actually detect if the compiler supports c++17, that will detect if it is the default or not.
I agree it would be better if we can detect it, but I haven't found any reasonable way of doing so besides doing something like grepping g++ -v --help (and I have no idea if that would work with clang)
There was a problem hiding this comment.
Hmmm, what if we used CXX_MAJOR and CXX_MINOR to figure that out? We know what version of gcc and clang will support c++17. If someone is using neither of those we can just still have the way to downgrade to C++14 if they have an old compiler.
Then the only users whose code fails will be people using old versions of compilers that are not gcc/clang
There was a problem hiding this comment.
I think some combination of TBB_CXX_TYPE, CXX_MAJOR, and CXX_MINOR could be used, yes. We'd need to know the exact values we want for each compiler
There was a problem hiding this comment.
I think it might be a bit more robust/portable to test for a feature macro for the corresponding support level
For example:
> echo | g++ -std=c++14 -x c++ -E -dM - | grep __cpp_aggregate_bases
> echo | g++ -std=c++17 -x c++ -E -dM - | grep __cpp_aggregate_bases
#define __cpp_aggregate_bases 201603LThere was a problem hiding this comment.
@WardBrian could you check out the most recent change I made to this PR? I added some checks for each compiler version and if it is a version we know supports c++17 we use that as the standard
There was a problem hiding this comment.
I think the basic idea would work, but I'm pretty sure the logic as you have it written leads to CXXFLAGS_LANG just being completely unset if the user has a compiler type we recognize but it's too old?
There was a problem hiding this comment.
I'm also not confident that the [ ] program works on Windows tbh
There was a problem hiding this comment.
I think the basic idea would work, but I'm pretty sure the logic as you have it written leads to CXXFLAGS_LANG just being completely unset if the user has a compiler type we recognize but it's too old?
Oh, yes!
It looks like the tests are running on windows for jenkins?
https://github.com/stan-dev/math/actions/runs/9116147777/job/25064095453?pr=3063#step:9:23
There was a problem hiding this comment.
I think the logic now is correct but overcomplicated. I would not set CXXFLAGS_PROGRAMS inside each if block, just set the HAS_CPP17 flag, and then at the end (pseudocode)
if HAS_CPP17
CXXFLAGS_PROGRAM ?= -std=c++17
else
CXXFLAGS_PROGRAM ?= -std=c++1y
if NOT STAN_USE_CPP14
warning()
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
@WardBrian do we want to merge this to include in this release? |
|
@SteveBronder I think we will need another round of RCs for other changes, so we can! Because I opened this PR I can't actually click 'approve', so if you can that would be great |
Summary
There have been discussions about moving to C++17 for a while. Stan compiles fine under
-std=c++17since #2693, and C++17 is already used by RStan.There so far hasn't been a push to actually use C++17 in our code base, and this is a proposal for the first step to do this:
-std=c++17in our Makefiles.STAN_USE_CPP14to switch back to-std=c++1y.That's it.
This is essentially designed to smoke out users who can't support C++17 while providing them with something they can put in
make/localto continue compiling for a version or two, until we actually add some C++17-requiring source code, at which point we delete the opt-out.I tried to find a more clever way to do this, but automatically detecting which standards a compiler supports seems to be a difficult/brittle thing to do.
Tests
None
Side Effects
Release notes
Stan now requests
-std=c++17. C++14 can still be used by defining the make variableSTAN_USE_CPP14. This variable will go away and C++17 will become a requirement in a future version.Checklist
Copyright holder: (fill in copyright holder information)
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested