Skip to content

Get rid of deprecation warnings#12

Draft
gsrohde wants to merge 3 commits intodevelopfrom
get_rid_of_deprecation_warnings
Draft

Get rid of deprecation warnings#12
gsrohde wants to merge 3 commits intodevelopfrom
get_rid_of_deprecation_warnings

Conversation

@gsrohde
Copy link
Copy Markdown

@gsrohde gsrohde commented Feb 13, 2024

This PR adds pragmas to two C++ source code files to suppress warnings about deprecated declarations in the Boost library code when C++17 is used for compilation. This will allow use to remove the C++11 SystemRequirement in client R packages.

@justinmcgrath
Copy link
Copy Markdown

The solution to this is to fix it in Boost. Half measures to work around problems in libraries add unnecessary work. I doubt we want to fix it, so we can report it to them if they haven't seen the problem already.

@gsrohde
Copy link
Copy Markdown
Author

gsrohde commented Feb 14, 2024

The solution to this is to fix it in Boost. Half measures to work around problems in libraries add unnecessary work. I doubt we want to fix it, so we can report it to them if they haven't seen the problem already.

This seemed to me a whole lot less work than our editing the Boost files, which is what we have been doing up to now when we have run into this sort of problem. And it seemed like a good, simple, short-term measure to circumvent package-check warnings while we wait who knows how long for Boost to remove C++17-deprecated features from their code.

Somewhat related to this, I happened upon the following in Section 1.6.4.2 ("C++17 Problems") of the Writing R Extensions guide:

R 4.3.0 and later default to C++17 when compiling C++, and that finally removed many C++98 features which were deprecated as long ago as C++11. Compiler/runtime authors have been slow to remove these, but LLVM clang with its libc++ runtime library finally started to do so in 2023 – some others warn but some do not.

The principal offender is the ‘Boost’ collection of C++ headers and libraries. There are two little-documented ways to work around aspects of its outdated code. One is to add

-D_HAS_AUTO_PTR_ETC=0
to PKG_CPPLAGS in src/Makevars, src/Makevars.win and src/Makevars.ucrt. This covers the removal of

std::auto_ptr
std::unary_function
std::binary_function
std::random_shuffle
std::binder1st
std::binder2nd
with most issues seen with code that includes boost/functional.hpp, usually indirectly.

[etc. etc.]

I was sort of hoping this would also fix the deprecated declaration issue, but it doesn't, though maybe there is similar, narrowly-targeted flag that would. (Using -Wno-deprecated-declarations to PKG_CPPLAGS seems a little heavy handed, since it would also turn off warnings about our own, non-Boost code.)

@justinmcgrath
Copy link
Copy Markdown

I don't think we should edit our Boost files. I think we should keep the C++11 requirement and wait for Boost to change their files. There is nothing wrong with our program (or with Boost for that matter), and we are making changes based an assumption that CRAN will reject this. There is nothing wrong with specifying a standard, and I would say that it is in fact good practice, in part because of things like this. These changes to not add features, improve performance or fix bugs. Since nothing is gained by changing the standard, we can wait until we want a change in one of those three categories that requires a new standard. If there were some conflict where we wanted a feature in C++17 then this would be a good way to update the standard and address the Boost warnings, but for now, what we have is exactly as good as what we'd be updating to.

@gsrohde
Copy link
Copy Markdown
Author

gsrohde commented Feb 16, 2024

I don't think we should edit our Boost files.

You're only bringing this up now? We've been doing this for a while.

But I think I basically agree with you until I hear a compelling reason not to. In any case, I'll take this PR off the table for now and will convert this (and biocro/biocro#86, which uses it) to a draft until and unless we need to do something further with this issue.

@gsrohde gsrohde marked this pull request as draft February 16, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants