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.

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

C++17/C++20 support #382

Closed
COM8 opened this issue May 20, 2020 · 12 comments
Closed

C++17/C++20 support #382

COM8 opened this issue May 20, 2020 · 12 comments

Comments

@COM8
Copy link
Member

COM8 commented May 20, 2020

We should update the complete library to be C++20 or at least C++17 ready.

My plan is to make use of C++17 features like std::opt and the new default deleter for std::unique_ptr and std::shared_ptr.
Besides that create a better RAII wrapper for curl sessions to properly support multithreaded access.
Perhaps even offer support for lambdas and coroutines.

Since it's rather early for C++20 features in such a library, this will be optional stuff and compilations should be possible with C++17 and C++20.

The old CPR (<C++17) will receive from then one only bug fixes. New features will only be added to the C++17 branche (master).

@zamazan4ik
Copy link

zamazan4ik commented May 26, 2020

I cannot understand the reason? Why? Do you want port whole CPR project to C++17 or even C++20? Remember, that a lot of users don't have even C++17 in their applications. So if you update the library to such new C++ standard, a lot of users will not be able to use CPR.

Do you need any specific stuff from C++17/20?

My personal point of view - that's acceptable to grade someday to C++17, but for now that's totally unacceptable grade the library to C++20 (since it's even officially released and C++ compilers have poor support for C++20). Please - don't upgrade cpr to C++20 now.

Thank you.

@Chlorie
Copy link

Chlorie commented May 26, 2020

I cannot understand the reason? Why? Do you want port whole CPR project to C++17 or even C++20? Remember, that a lot of users don't have even C++17 in their applications. So if you update the library to such new C++ standard, a lot of users will not be able to use CPR.

Do you need any specific stuff from C++17/20?

My personal point of view - that's acceptable to grade someday to C++17, but for now that's totally unacceptable grade the library to C++20 (since it's even officially released and C++ compilers have poor support for C++20). Please - don't upgrade cpr to C++20 now.

Thank you.

I think what they meant was to make CPR better integrated with C++17/20 if your build settings support those, like with version detection macros so that if you use C++17/20 CPR will support new fancy things (like coroutines?) but if your compiler don't have the support those parts of the library are simply not accessible (disabled by preprocessor). I don't see how this will be a problem for people using older compilers.

@COM8
Copy link
Member Author

COM8 commented May 26, 2020

My plan is to make use of C++17 features like std::opt and the new default deleter for std::unique_ptr and std::shared_ptr.
Besides that create a better RAII wrapper for curl sessions to properly support multithreaded access.
Perhaps even offer support for lambdas and coroutines.

Since it's rather early for C++20 features in such a library, this will be optional stuff and compilations should be possible with C++17 and C++20.

The old CPR (<C++17) will receive from then one only bug fixes. New features will only be added to the C++17 branche (master).

@COM8 COM8 added this to the CPR 1.6.0 milestone Jul 8, 2020
@HellsingDarge
Copy link

Also being able to pass string_view into cpr::Url and other applicable locations would also be nice

@COM8
Copy link
Member Author

COM8 commented Aug 31, 2020

Also being able to pass string_view into cpr::Url and other applicable locations would also be nice

Yes, good idea!

@xloem
Copy link
Contributor

xloem commented Sep 23, 2020

I cannot understand the reason? Why? Do you want port whole CPR project to C++17 or even C++20? Remember, that a lot of users don't have even C++17 in their applications. So if you update the library to such new C++ standard, a lot of users will not be able to use CPR.
Do you need any specific stuff from C++17/20?
My personal point of view - that's acceptable to grade someday to C++17, but for now that's totally unacceptable grade the library to C++20 (since it's even officially released and C++ compilers have poor support for C++20). Please - don't upgrade cpr to C++20 now.
Thank you.

I think what they meant was to make CPR better integrated with C++17/20 if your build settings support those, like with version detection macros so that if you use C++17/20 CPR will support new fancy things (like coroutines?) but if your compiler don't have the support those parts of the library are simply not accessible (disabled by preprocessor). I don't see how this will be a problem for people using older compilers.

Just to note, it does look like the community disagrees a little here. COM8 seemed to be proposing to stop providing feature updates for c++11/c++14, possibly due to the limits of their capacity being maintainer. I wonder if there is enough community support to sustain maintenance for older compilers, while still offering features for new. Could others possibly step in to backport and forward-port features, or sort out preprocessor defines? If not, we could always propose that one of the variants become a fork.

@acidicoala
Copy link

C++ 17 support is wholeheartedly supported, because for now we have to suppress CXX17 deprecation warnings, that prevent compilation on MSVS.

@shyney7
Copy link

shyney7 commented Apr 17, 2021

Please dont make C++20 mandatory. Many systems still dont have a c++20 compiler and it gets even worser if you do cross-compilation for specific embedded systems.

@zydxhs
Copy link
Contributor

zydxhs commented Jul 27, 2021

I have never used C++20, and I will not consider C++20 for a long time.
C++17 is quite new and acceptable.

@simon-berger
Copy link
Contributor

I would like to work on this issue. My plan is to work out a concept first and then update CPR with as few compatibility restrictions as possible.

@simon-berger
Copy link
Contributor

simon-berger commented Jun 15, 2022

Before I start to work on a PR I would like to discuss some of my ideas.

As far as I understand, the plan is to use C++17 as default (required) and if needed support optional C++20 compilation to enable certain special features. Accordingly, I would increase the CMake CXX_STANDARD to 17 first and consider C++20 only after C++17 has been integrated.

Here is a tentative list of potential improvements / features that I have in mind to implement as part of the update to C++17:

  • Implement std::string_view support (e.g. for cpr::Url as proposed by @HellsingDarge)
  • Improve lambda functions:
    • Type determination with auto
    • Initialization of function-local constants directly in captures
  • Use of std::optional (e.g. for ranges which are only partially defined)
  • Use the new filesystem library
  • Use std::byte instead of char / unsigned char
  • Support std::chrono_literals for timeouts
  • Use auto as return type for functions without decltype
  • Use of make_unique()
  • Make cpr::Session thread safe using std::scoped_lock
  • Investigate lambda capture of *this (might be interesting for async requests)
  • ...

@COM8 can you briefly explain me what you meant by that:

My plan is to make use of C++17 features like [...] the new default deleter for std::unique_ptr and std::shared_ptr.

Feedback is very appreciated :)

@COM8
Copy link
Member Author

COM8 commented Jun 15, 2022

Yes. cpr 1.9.0 won't change anything in regards to the minimum required version of C++. BUT for 1.10.0 we will increment it to cpp17 with optional cpp20 features (CMake option).

All your suggestions sound good. Only making cpr::Session thread safe is not needed in my eyes, since using a session in parallel is a broken concept in my eyes.

Regarding the default deleter:
This already has been fixed, when we moved the code to cpp11. There default_delete got introduced. Therefore this is no concern any more.

@simon-berger simon-berger mentioned this issue Jun 20, 2022
14 tasks
@COM8 COM8 closed this as completed Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants