Added support for string_view in C++17 #1028
Conversation
|
Hey @gracicot, thanks for the PR! |
| { | ||
| if (JSON_UNLIKELY(not j.is_string())) | ||
| { | ||
| JSON_THROW(type_error::create(302, "type must be string, but is " + std::string(j.type_name()))); |
There was a problem hiding this comment.
Is there a test case checking for this exception? If not, please add one.
|
There is an issue with MSVC 2017 using |
|
Sorry for the delay. I haven't investigated the issue with visual studio yet, I will be soon. |
|
Any ideas on how to fix this? |
|
I investigated and it lead me to think it's linked to #464 . This is a MSVC bug, and If I look at cppreference, an overload of I haven't found a solution yet that would allow implicit conversions to |
|
I think disabling it for MSVC for the time being could be a way to go. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
state: stale
stale
bot
removed
the
state: stale
|
So the bug preventing enabling this feature on msvc has been fixed: https://developercommunity.visualstudio.com/content/problem/243183/stdstring-should-not-have-a-operator-that-takes-a.html It should be out in 15.8 (msvc 19.15?), so in 19.14 and older it is disabled, as discussed. |
|
|
||
| template<typename BasicJsonType, typename CompatibleStringType, | ||
| enable_if_t<not std::is_same<CompatibleStringType, typename BasicJsonType::string_t>::value, | ||
| int> = 0> |
There was a problem hiding this comment.
This SFINAE check is already done in the to_json method, maybe we can only do it once:
- Adding a
to_json(const BasicJson::string_t&)overload (onlyBasicJson::string_t&&is handled right now) - Then, in the
to_jsonoverload where thestd::is_constructible<...>check is performed, construct astring_tand move it to theexternal_constructor.
Additionally, I'd rather keep external_constructor function count low (Maybe the array_t specialization could get cleaned a bit).
This is mostly a style preference though.
| not std::is_same<typename BasicJsonType::string_t, | ||
| CompatibleStringType>::value and | ||
| std::is_constructible < | ||
| BasicJsonType, typename CompatibleStringType::value_type >::value, |
There was a problem hiding this comment.
Is this check relevant?
There was a problem hiding this comment.
You are right, it's not necessary. I also missed a check elsewhere, let me fix that.
|
I'm on holidays, but I will have a look at this PR once I am back at the end of June. |
This comment has been minimized.
This comment has been minimized.
|
Thanks a lot! |
This patch adds support for
std::string_viewconversion (explicit and implicit). It also added construction fromstd::string_view.I did that by following how many compatible array types were handled and adding a type trait that checks for compatible string types.
However, I fear the trait is kinda loose in which type is allows (see
is_compatible_string_type). Tell me if it needs to be fixed!