to_string(): replace template with 6 overloads, simplify #76
+293
−95
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As mentioned in the code comment in `kaitai/kaitaistream.cpp`, this approach follows what `std::to_string` is doing: https://en.cppreference.com/w/cpp/string/basic_string/to_string Avoiding templates and separating signed and unsigned cases has been already suggested by @webbnh in #50. The motivation for this change is that this new version of `to_string` also accepts values of unscoped enums that can be implicitly converted to integers, just like `std::to_string` does. For example, you can do `kaitai::kstream::to_string(FRUIT_APPLE)` just like you can do `std::to_string(FRUIT_APPLE)`, where `FRUIT_APPLE` is an unscoped enum member: ```cpp enum fruit_t { FRUIT_APPLE = 2, FRUIT_ORANGE = 5 }; ``` This simplifies the translation of expressions like `fruit::apple.to_i.to_s`, because it means that the `enum.to_i` operation can remain a no-op in the KSC: [`CppTranslator.scala:193-194`](https://github.com/kaitai-io/kaitai_struct_compiler/blob/12dbc32226eb9e94b76f803f3c8ff5f8943e5f8d/shared/src/main/scala/io/kaitai/struct/translators/CppTranslator.scala#L193-L194) ```scala override def enumToInt(v: expr, et: EnumType): String = translate(v) ``` Therefore, this change fixes the [EnumToI](https://github.com/kaitai-io/kaitai_struct_tests/blob/af0359aeb87233640fcd85b57850c712b6f03f63/formats/enum_to_i.ksy) and EnumToIInvalid tests for C++11, which currently fail with this error (https://github.com/kaitai-io/ci_artifacts/blob/786a45a62978409fd90fab124b5b1ad1f225a81e/test_out/cpp_stl_11/build-1.log#L1495-L1498): ``` /tests/compiled/cpp_stl_11/enum_to_i.cpp: In member function 'std::string enum_to_i_t::pet_1_i_to_s()': /tests/compiled/cpp_stl_11/enum_to_i.cpp:65:48: error: no matching function for call to 'kaitai::kstream::to_string(enum_to_i_t::animal_t)' 65 | m_pet_1_i_to_s = kaitai::kstream::to_string(pet_1()); | ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~ ``` Eventually, we might switch to scoped enums (see kaitai-io/kaitai_struct#959), which are not implicitly convertible to integers, so then it won't be possible to keep the `.to_i` operation implemented as a no-op, but for now (as long as we're using unscoped enums) it works.
Otherwise we get errors like this when compiling with `-std=c++98`:
```
In file included from /home/runner/work/kaitai_struct_cpp_stl_runtime/kaitai_struct_cpp_stl_runtime/kaitai/kaitaistream.cpp:1:
/home/runner/work/kaitai_struct_cpp_stl_runtime/kaitai_struct_cpp_stl_runtime/kaitai/kaitaistream.h:250:39: error: ISO C++ 1998 does not support ‘long long’ [-Werror=long-long]
250 | static std::string to_string(long long val);
| ^~~~
```
If we always use [GoogleTest v1.14.0](https://github.com/google/googletest/releases/tag/v1.14.0) (which declares that it requires at least C++14) even for `-DCMAKE_CXX_STANDARD=98`, then only `kaitaistream.cpp` gets a `-std=c++98` option, while `unittest.cpp` is compiled under `-std=c++14`. This is a problem, because then neither `tests/unittest.cpp` nor `kaitai/kaitaistream.h` included from `tests/unittest.cpp` know that the `kaitaistream` library was compiled without C++11 support and try to use it, which doesn't work. Not to mention that such mixing of C++ standards doesn't make much sense if we want to verify that we are compatible with a particular version of the C++ standard.
|
Hi Guys! Cool to see that you are still working on this stuff. :-) This change looks pretty nice! |
This is more foolproof, because it allows you to compile the `kaitaistream.cpp` library under `-std=c++98` and link to it from an application compiled under `-std=c++11` (and everything will still work fine). This was not possible before, see 4bc5896.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As mentioned in the code comment in
kaitai/kaitaistream.cpp, this approach follows whatstd::to_stringis doing:https://en.cppreference.com/w/cpp/string/basic_string/to_string
Avoiding templates and separating signed and unsigned cases has been already suggested by @webbnh in #50.
The motivation for this change is that this new version of
to_stringalso accepts values of unscoped enums that can be implicitly converted to integers, just likestd::to_stringdoes. For example, you can dokaitai::kstream::to_string(FRUIT_APPLE)just like you can dostd::to_string(FRUIT_APPLE), whereFRUIT_APPLEis an unscoped enum member:This simplifies the translation of expressions like
fruit::apple.to_i.to_s, because it means that theenum.to_ioperation can remain a no-op in the KSC:CppTranslator.scala:193-194Therefore, this change fixes the EnumToI and EnumToIInvalid tests for C++11, which currently fail with this error (https://github.com/kaitai-io/ci_artifacts/blob/786a45a62978409fd90fab124b5b1ad1f225a81e/test_out/cpp_stl_11/build-1.log#L1495-L1498):
Eventually, we might switch to scoped enums (see kaitai-io/kaitai_struct#959), which are not implicitly convertible to integers, so then it won't be possible to keep the
.to_ioperation implemented as a no-op, but for now (as long as we're using unscoped enums) it works.