feat: support enums in D-Bus serialization and signatures #416
Conversation
a test was not discovered, because the tests were run with c++17 I set the cxx_standard for the tests explicitly, so it will be detected in the future
currently only useable if you have a c++20 compiler, but probably can be fixed if needed
|
This is some weird bug: actions/runner-images#9446, but nothing that strikes the user.
|
There was a problem hiding this comment.
Hey @hellow554, thank you for the PR, makes a lot of sense to also support enums in a generic way. I left a few comments...
include/sdbus-c++/Message.h
Outdated
|
|
||
| template <typename Enum> requires std::is_enum_v<Enum> | ||
| Message& operator<<(const Enum &item) { | ||
| return operator<<(static_cast<std::underlying_type_t<Enum>>(item)); |
There was a problem hiding this comment.
Please follow the style and only declare the function here and define it below the class. Also, please follow the established style with the positioning of the opening curly brace. (ditto for operator>>)
There was a problem hiding this comment.
Also, you can then move these functions out of C++20 #ifdef scope...
|
Now I noticed in your description that it's OK for you if I edit your PR. I requested a number of changes... shall I do them in this PR? |
|
Please go ahead and feel free to edit it. Do you want me to do things or should I justed accept your requests? |
|
@hellow554 I'm doing the changes already... will have it in a few minutes. |
sangelovic
changed the title
|
@hellow554 What do you think of my changes? I've implemented C++17 solution but also kept C++20 one of yours. |
|
It took me more than a few minutes as I expected at the beginning... I had an illogical compiler error with |
tests/CMakeLists.txt
Outdated
| @@ -107,12 +107,10 @@ add_executable(sdbus-c++-unit-tests ${UNITTESTS_SRCS}) | |||
| target_compile_definitions(sdbus-c++-unit-tests PRIVATE | |||
| LIBSYSTEMD_VERSION=${LIBSYSTEMD_VERSION} | |||
| SDBUS_HEADER=<${LIBSYSTEMD_IMPL}/sd-bus.h>) | |||
| target_compile_features(sdbus-c++-unit-tests PUBLIC cxx_std_20) | |||
There was a problem hiding this comment.
This way you can never test any c++20 dependent code again.
What if we use try_compiles to check if c++20 is supported and if yes, we add a second target with just std=c++20 enabled? So we have 4 tests instead of 2? (or 3, if we leave out the intregation tests)
There was a problem hiding this comment.
OK, I thought again, and I'm fine with putting that line back. (I don't want to complicate things with try_compiles and additional targets.)
But, again, there is v2.0 about to be released which officially supports C++20. v2 will become the main line where the main focus will be onwards, v1 will be kept for bugfixing and maintenance. With that, we'll have C++20 library and tests covered with compilers that supports C++20.
Is it OK for you if we leave it out here and the things will work automatically with v2?
There was a problem hiding this comment.
Nevermind, I brought the cxx20 lines back, and I will leave them out in release/v2.0 when rebasing.
|
Looks really good to me. Thanks for the quick solution provided by you. Thanks again! |
|
I tried to fix the pipeline but there's more to it than I first anticipated. I don't have time for it, and anyways it's out of scope of this PR. So I reverted the cxx20 feature for the tests. I think it's fully OK that it's covered in the upcoming |
This PR adds the possiblity for C++20 compilers to use enums, enum classes and enum structs.
C++20 is required because of
requires. I coulndn't find a way without it. If you're more successful than me, please open a PR or edit this PR, feel free :)If anything bugs you, please comment