C++20 compatibility #1226
C++20 compatibility #1226
Conversation
It was standardized in C++20 and thus not needed. Also it causes compilation issues with newer stdlib.
This behaviour is deprecated since C++20
Capture by-reference is probably what was intended from the start, seeing how the buffer is passed.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1226 +/- ##
==========================================
- Coverage 78.49% 76.49% -2.00%
==========================================
Files 52 58 +6
Lines 6282 9668 +3386
==========================================
+ Hits 4931 7396 +2465
- Misses 1351 2272 +921 ☔ View full report in Codecov by Sentry. |
| #include <chrono> | ||
| int main() { | ||
| std::stringstream ss; | ||
| ss << std::format("%FT%T%z", std::chrono::system_clock::now()); |
There was a problem hiding this comment.
I would avoid using std::format, since it isn't well supported everywhere and we're just checking for C++20 chrono anyway. Also, the code isn't ran, so you can simply call the functions you want to check for, without actually writing something that "makes sense".
There was a problem hiding this comment.
If we want to replace date::to_stream from hinnant-date we kinda have no other option. This is the way to turn dates into strings in C++20, no way around it. The fmt lib is also available as a sort of preview of C++20 std::format like hinnant-date is a preview of the date changes. So a fallback could be introduced, but it would be a rare cornercase for something that is already a rare cornercase (building pistache for C++20).
|
Il giorno mar 6 ago 2024 alle 11:13:16 -07:00:00, George Sedov
***@***.***> ha scritto:
If we want to replace date::to_stream from hinnant-date we kinda have
no other option
Makes sense, I wasn't thinking about the new date::to_stream function
you've introduced. It's OK to keep this as is then :)
|
src/client/client.cc
Outdated
| @@ -186,7 +186,7 @@ namespace Pistache::Http::Experimental | |||
| void registerPoller(Polling::Epoll& poller) override; | |||
| void unregisterPoller(Polling::Epoll& poller) override; | |||
|
|
|||
| Async::Promise<void> asyncConnect(std::shared_ptr<Connection> connection, | |||
| Async::Promise<void> asyncConnect(const std::shared_ptr<Connection>& connection, | |||
There was a problem hiding this comment.
Is this related to the modified lambda captures? If not, please either split this in a separate commit explaining why this is needed or drop this change
There was a problem hiding this comment.
Hm, no, it's just an error. When I zoned out for a little and just started pressing "yes" to the CLion suggestions to fix things.
While the project is aiming at C++17, it should be possible to use it with newer versions of the standard. There are two problems preventing it.
First is the hinnant-date library. It was included in the C++20 standard, but with quirks, so using it as-is is problematic. On some compilers it just fails. This PR introduces the wrapper, which replaces the lib with the standard found in
std::chronoif the language version is high enough.Second is the implicit lambda capture of
this. It is deprecated since C++20, you are supposed to use[=, this]instead. The problem is, this syntax is only introduced in C++20, and it doesn't work in the previous versions. The additional problem is that in pistache the[=]capture is used everywhere, even when it doesn't make any sense. For example in the Transport::asyncWrite function we capturebufferby value (copy the content once), and then pass it to theBufferHolderconstructor which copies it again. I tried to understand the logic behindBufferHolderbut I couldn't. Why doesBufferHolder::detachcopies the content of the buffer before passing it to the constructor that copies it again? And same with theConnection, why do you use refs in a lambda that captures everything by-value? Argh! My head hurts! But anyway, this is outside the scope of this PR. I replaced the implicit captures with explicit ones in places where it caused problems and tried to keep everything exactly as-is. Fixing the memory management should be a task for somebody more qualified than me.