Uh oh!
There was an error while loading. Please reload this page.
Use C++20. - #2322
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## nightly #2322 +/- ##
==========================================
- Coverage 6.17% 6.05% -0.13%
==========================================
Files 86 86 Lines 17542 17536 -6 Branches 8179 8184 +5 ==========================================
- Hits 1083 1061 -22 - Misses 15362 15434 +72 + Partials 1097 1041 -56
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AppImage build fails because Ubuntu 20.04 provides only Boost 1.71, which is incompatible with C++20. https://stackoverflow.com/questions/62723053/boost-log-expressions-v1-71-will-not-compile-under-c20 There is a more recent LTS version. Could we drop 20.04? Every other distribution that Sunshine makes builds for provides 1.74 or higher, even Debian 11. |
What wonderful timing! |
tez011
commented
Mar 31, 2024
depends on #2327 |
ReenigneArcher
commented
Apr 4, 2024
Merged. |
| ss << std::hex << std::setfill('0'); | ||
| for (const auto &ch : str) { | ||
| ss << ch; | ||
| ss << static_cast<uint_least32_t>(ch); |
There was a problem hiding this comment.
char32_t — type for UTF-32 character representation, required to be large enough to represent any UTF-32 code unit (32 bits). It has the same size, signedness, and alignment as std::uint_least32_t, but is a distinct type.
https://en.cppreference.com/w/cpp/language/types#char32_t
Therefore we should expect no data loss with a static_cast to uint_least32_t
| auto file = entry.path().filename(); | ||
| auto filestring = file.generic_u8string(); | ||
| auto filestring = file.generic_string(); |
There was a problem hiding this comment.
generic_u8string now returns basic_string<char8_t>. since char8_t is unsigned, this is now incompatible with std::string_view = std::basic_string_view<char>. Assuming this goes in with the UCRT64 change our locale is not exactly guaranteed to be utf-8 either, and the comparison we're immediately making is to an ANSI string so the u8-ness seems mostly irrelevant here. Therefore, making this change so compilation succeeds.
There was a problem hiding this comment.
Good thing is that we don't use file.generic_string(); on Windows. Yet...
tez011
commented
Apr 5, 2024
Is there anything else in this pull request I should handle before it's able to be merged? |
ReenigneArcher
commented
Apr 5, 2024
Just needs testing and approval. It will be after v0.23.0. |
tez011
commented
Apr 20, 2024
Hi! Just waiting for testing/approval results for this pull request. This first change is fairly small so I'm hoping there aren't too many obstacles! |
@tez011 sorry for the delays. We have a lot of open PRs. It might take a little bit before the other approvals come in. I think we are going to try to put out another patch to get a few things fixed before merging these bigger ones. In the meantime, can you rebase your branch on top of the latest nightly? |
| set_target_properties(sunshine PROPERTIES CXX_STANDARD 20 | ||
| VERSION ${PROJECT_VERSION} | ||
| SOVERSION ${PROJECT_VERSION_MAJOR}) | ||
There was a problem hiding this comment.
Just below we have:
target_compile_options(sunshine PRIVATE $<$<COMPILE_LANGUAGE:CXX>:${SUNSHINE_COMPILE_OPTIONS}>;$<$<COMPILE_LANGUAGE:CUDA>:${SUNSHINE_COMPILE_OPTIONS_CUDA};-std=c++17>)
This either need to be changed to c++20 or removed? @ReenigneArcher do you know why we need to specify the standard explicitly again?
^ This includes here and in other files.
There was a problem hiding this comment.
I settled on this change after trying both options you describe. The cuda compiler doesn't support c++20.
There was a problem hiding this comment.
I'm not sure why it was originally specified explicitly. I guess because it would fail to compile if the wrong standard was used?
There was a problem hiding this comment.
I'm looking to bump to c++23 in #4096 and came here just to see what all was changed when bumping to 20. It looks like the bump to 23 will be quite a bit easier than the one to 20, as everything is just building right away.
It seems that with cuda we can specify the standard directly in the file name, though I'm not sure that's a good idea? https://docs.nvidia.com/cuda/cuda-compiler-driver-nvcc/index.html#std-c-03-c-11-c-14-c-17-c-20-std
It also seems that at least recent versions of cuda now support c++20. Perhaps this can be set dynamically.
Uh oh!
There was an error while loading. Please reload this page.
| ss << std::hex << std::setfill('0'); | ||
| for (const auto &ch : str) { | ||
| ss << ch; | ||
| ss << static_cast<uint_least32_t>(ch); |
There was a problem hiding this comment.
Why are we casting? The char32_t is already unsigned.
There was a problem hiding this comment.
stringstreams don't approve of mixing type char and type char32_t. (You can try this.) This cast is done to force the stringstream to recognize that this value is a 32-bit integer and to hex it accordingly.
There was a problem hiding this comment.
/home/runner/work/Sunshine/Sunshine/src/platform/linux/input.cpp: In function ‘std::string platf::to_hex(const std::__cxx11::basic_string<char32_t>&)’:
/home/runner/work/Sunshine/Sunshine/src/platform/linux/input.cpp:1513:13: error: use of deleted function ‘std::basic_ostream<char, _Traits>& std::operator<<(std::basic_ostream<char, _Traits>&, char32_t) [with _Traits = std::char_traits<char>]’
1513 | ss << ch;
| ^~
In file included from /usr/include/c++/10/bits/unique_ptr.h:42,
from /usr/include/c++/10/bits/locale_conv.h:41,
from /usr/include/c++/10/locale:43,
from /usr/include/boost/locale/boundary/facets.hpp:17,
from /usr/include/boost/locale/boundary.hpp:12,
from /usr/include/boost/locale.hpp:11,
from /home/runner/work/Sunshine/Sunshine/src/platform/linux/input.cpp:21:
/usr/include/c++/10/ostream:553:5: note: declared here
553 | operator<<(basic_ostream<char, _Traits>&, char32_t) = delete;
| ^~~~~~~~
make[2]: *** [tests/CMakeFiles/test_sunshine.dir/build.make:665: tests/CMakeFiles/test_sunshine.dir/__/src/platform/linux/input.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/Makefile2:271: tests/CMakeFiles/test_sunshine.dir/all] Error 2
| auto file = entry.path().filename(); | ||
| auto filestring = file.generic_u8string(); | ||
| auto filestring = file.generic_string(); |
There was a problem hiding this comment.
Good thing is that we don't use file.generic_string(); on Windows. Yet...
| * @brief todo | ||
| */ | ||
| #pragma once | ||
| #include <utility> |
There was a problem hiding this comment.
There are a lot of includes for this. Are they really needed? Especially here, in the header.
There was a problem hiding this comment.
It seems they are needed before any boost includes, otherwise a lot of errors like this are observed:
https://github.com/tez011/Sunshine/actions/runs/8484576885/job/23247793427
If we can include a boost header in here, we can certainly include a system header above it, no?
There was a problem hiding this comment.
No problems with that, it's just that currently the "includes" state in Sunshine is a mess. For example, in this file even we include asio, but it not used anywhere in the header...
But that's out of scope for this PR.
FrogTheFrog
commented
Apr 26, 2024
@ReenigneArcher LGTM |
Description
#2149 was reverted in #2320 and needs to be split into three changes:
Type of Change
.github/...)Checklist
Branch Updates
LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.