Update C++ Standard to C++17

[andrjohns]

As discussed over on the forums, CRAN has begun issuing a NOTE when the requested C++ standard is lower than C++17. This PR updates the rstantools-provided Makevars to request the higher standard.

I've run reverse-dependency checks and all packages continue to build and pass. This will break the current rstan 2.26, but I've already opened a PR with the needed patches here

[jgabry]

Thanks @andrjohns! I've tagged @bgoodri to review since this is more in his wheelhouse than mine.

[bgoodri]

Can we run the upstream unit tests under the C++-17 standard once?

[andrjohns]

Can we run the upstream unit tests under the C++-17 standard once?

Which unit tests do you mean sorry?

[bgoodri]

I meant all of them in Stan Math and Stan Library but with -std=c++-17 in the makefile.

[jgabry]

@andrjohns Also, at this point we should make you a coauthor on this package instead of just a contributor. Feel free to change ctb to aut in the DESCRIPTION file as part of this PR.

Also feel free to make branches directly in the repo in the future (if you want). I think you should have permission but if not let me know.

[andrjohns]

I meant all of them in Stan Math and Stan Library but with -std=c++-17 in the makefile.

Ah good point. I made branches of the Math and Stan versions used in rstan 2.21 and added an actions workflow to run the respective tests under std=gnu++-17, all tests passed:

• Math
• Stan

[andrjohns]

And the upstream tests with recent Stan & Math were also run under c++-17 as part of this PR

[jgabry]

@bgoodri Does this change look ok to you? If so I can start preparing the CRAN submission. On the forum @andrjohns said he ran reverse dependency checks and all packages passed.

Not sure if you saw this, but the reason for the approach @andrjohns took is related to this post from user @ edm on the forum (I think username @ecmerkle on GitHub):

Related to this C++17 issue: I uploaded a new package version to CRAN a few days ago and today received the message below. I am reporting back in case it influences the PR. (I edited the message to only include relevant parts; I believe @phcooney also received it)

These are now being compiled with C++17. Unfortunately they make use of
Boost headers that are not compliant with that standard, using functions
which were deprecated in C++11 and removed in C++17.

One workaround is to declared a dependence on C++14

Reading the headers suggested

PKG_CPPFLAGS = -D_HAS_AUTO_PTR_ETC=0

in src/Makevars[.win] would suffice, and I tested that for DDD (already
reported). Or you could add

#if __cplusplus >= 201703L
# define _HAS_AUTO_PTR_ETC 0
#endif

at the top of the files using Boost headers. (It looks like the stan
users should get that done upstream.)

[cjvanlissa]

Dear all, is there any progress on this? I'm holding back a necessary update to a package that depends on rstan and rstantools because of the aforementioned CRAN note.

[wds15]

I concur in that this PR should move ahead ASAP, since all downstream packages really need this upgrade. I just had to workaround things in an ugly way...see here

https://github.com/weberse2/RBesT/blob/1bb60a01f37cdcc613d752a438f16d4f19b5760d/configure

To me it appears that CRAN is now happy with C++17 being used as a new default C++ standard. I have verified with a docker image (rhub/clang16)

https://hub.docker.com/u/rhub

that the fix which is implemented in this PR is indeed what resolves the issues with clang 16 (this extra define thing).

However, when reading the CRAN instructions for package maintainers carefully, they also say that it is strongly recommended to add C++17 to the SystemRequirements field of the DESCRIPTION file whenever C++17 is used (in addition to the src/Makevars). I am not sure if this is a hard check on their end on this, but their manuals imply that one should have this. Maybe even that could be automated with rstantools (that is to add C++17 automatically if no C++ standard has been set)? Or we just leave this.

In any case... this PR would be great to roll out soon.

However, making C++17 the default could be a challenge for some packages which use functions like "size" as I recall. These have compile issues with C++17. Not sure what to do about this other than finally moving to 2.26 where the C++17 back ports have been incorporated as I understood from @andrjohns .

BTW... when building rstan within the clang16 rhub docker image I was forced to define BOOST_ALLOW_DEPRECATED to make things work. If I did not set this, then some phoenix stuff would trigger a compile error. This deprecation happened in the parser bits and pieces of rstan 2.21, which I assume we would get rid of with 2.26

[andrjohns]

However, making C++17 the default could be a challenge for some packages which use functions like "size" as I recall. These have compile issues with C++17.

This isn't an issue for rstan/StanHeaders 2.21 since the templating for the Math library's size() function at the time was not as general. StanHeaders 2.26 has also been patched for handling this with both rstan 2.21 (this PR) and rstan 2.26 (this PR).

BTW... when building rstan within the clang16 rhub docker image I was forced to define BOOST_ALLOW_DEPRECATED to make things work.

I believe this has been addressed by adding -D_HAS_AUTO_PTR_ETC=0 to the PKG_CPPFLAGS in the current PR, as was recommended by CRAN

[wds15]

Let's see, maybe Dirk can handle the matter with C++17 upstream in BH:

eddelbuettel/bh#92