First draft of cetl::pf17::string_view
#142
Conversation
serges147
commented
Oct 17, 2024
•
serges147
commented
- First pr for issue Provide cetl::string_view #2 . Not all c++17 overloads are implemented but most of basic ones are there.
|
|
||
| enum : size_type | ||
| { | ||
| npos = static_cast<size_type>(-1) |
There was a problem hiding this comment.
Why not static constexpr?
There was a problem hiding this comment.
c++14 and ODR issue
Could you please show me an example if you know a nicer syntax which is compatible with c++14.
| @@ -0,0 +1,323 @@ | |||
| /// @file | |||
There was a problem hiding this comment.
It's critical that we start with type-parameterized tests that compare the CETL implementation to the ones in our representative standard libraries like https://github.com/OpenCyphal/CETL/blob/main/cetlvast/suites/unittest/test_pf20_span.cpp does for span.
There was a problem hiding this comment.
- See below my response, namely:
My thinking was that it will add more work while we most probably will use char specialization only.
Could you please elaborate why it is critical to invest into what we most probably (plz correct me if I'm wrong) will not use? Are we making CETL universal for the whole world? Honestly, it's quite exhausting to "fight" against c++14 - I believe libcyphal were long time done already if it was not support of 14 standard. And Nunavut's c++ code generation goals will be much simpler and more clean. I do understand that anyway there will be always a "race" against the latest standard and what we "have to support", so polyfills will probably still live in some form... it just concerns me (a lot!) that a lot of simple things become not so simple, and hardly contribute to the final product we deliver to customers. Sorry, for my frustration and being emotional ;)
- BTW, you said
... compare the CETL implementation to the ones in our representative standard libraries ..."
I didn't quite catch what you meant by "compare"... I don't see there any comparison of what cetl::pf20::span and std::span does... if this is what you meant.
Anyway, if you confirm (and hopefully explain why) that basic_string_view is a must then of course I will change my tests to be type parameterized. TBD: all of the below?
Defined in header [<string_view>](https://en.cppreference.com/w/cpp/header/string_view)
Type Definition
std::string_view (C++17) std::basic_string_view<char>
std::wstring_view (C++17) std::basic_string_view<wchar_t>
// std::u8string_view (C++20) std::basic_string_view<char8_t> // TBD: should be under our pf20?
std::u16string_view (C++17) std::basic_string_view<char16_t>
std::u32string_view (C++17) std::basic_string_view<char32_t>
There was a problem hiding this comment.
The simple answer is that CETL's polyfill types are re-implementations from the standard and the standard defines basic_stringview then provides aliases. We can tolerate sparse implementations but must minimize deviant implementations.
As for the reasoning for C++14? It just goes to the state of the world and compatibility. The need for C++14 polyfill types will naturally diminish over time just as all software gets outdated and deprecated as the years roll by.
include/cetl/pf17/string_view.hpp
Outdated
| namespace pf17 | ||
| { | ||
|
|
||
| class string_view |
There was a problem hiding this comment.
In the standard, string_view is just an alias for basic_string_view so we need to start with this template.
There was a problem hiding this comment.
ok, I can do it. My thinking was that it will add more work while we most probably will use char specialization only.
You are right, I for some reason thought that we don't duplicate docs (cppreference) for our polyfills, but now I see that span, variant and etc do contain certain level of documentation - I'll fix it. |
|
|
Well, that didn't go as I had planned. I pushed an example of how to write these A/B tests but it ended up "merging" your PR into this repo's new string_view_2 branch. Can you take a look and finish your work up on that branch or pull it over and reopen this one? Sorry. |
