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.

Sign up for GitHub

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

Jump to bottom

Request C++17 by default, add fallback flag for now #3063

Merged
merged 5 commits into from
May 21, 2024
Merged

Request C++17 by default, add fallback flag for now #3063

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
07cb7ab
Request C++17 by default, add fallback flag for now
WardBrian May 6, 2024
9b71558
update to use logic to check compiler version for c++17 compatability
SteveBronder May 16, 2024
e43440c
update to use logic to check compiler version for c++17 compatability
SteveBronder May 16, 2024
a4bc3cd
fix type where HAS_CXX17 should be false for ifeq failure
SteveBronder May 16, 2024
16337a1
cleanup make definitions for c++17
SteveBronder May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions make/compiler_flags
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,12 @@ CPPFLAGS_SUNDIALS ?= -DNO_FPRINTF_OUTPUT $(CPPFLAGS_OPTIM_SUNDIALS) $(CXXFLAGS_F
#CPPFLAGS_GTEST ?=


## setup compiler flags
CXXFLAGS_LANG ?= -std=c++1y
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
Copy link
Collaborator

@SteveBronder SteveBronder May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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
endif

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

@SteveBronder SteveBronder May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 201603L

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also not confident that the [ ] program works on Windows tbh

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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()

#CXXFLAGS_BOOST ?=
CXXFLAGS_SUNDIALS ?= -pipe $(CXXFLAGS_OPTIM_SUNDIALS) $(CPPFLAGS_FLTO_SUNDIALS)
#CXXFLAGS_GTEST
Expand Down