Enable using string views with wil::str_concat #590
Conversation
| return str_raw_ptr(val.get()); | ||
| } | ||
|
|
||
| struct string_view_t |
There was a problem hiding this comment.
Is the intent of this being "public" so that third parties can overload without touching the details namespace?
There was a problem hiding this comment.
I suppose this is probably just following str_raw_ptr. Searching for def:str_raw_ptr in the OS tree yields no results outside of the WIL source code, so that might be enough to conclude that this doesn't necessarily need to be made public.
| return str.c_str(); | ||
| } | ||
|
|
||
| inline string_view_t view_from_string(std::wstring_view const& s) |
There was a problem hiding this comment.
This should get moved down to be inside the #if __cpp_lib_string_view >= 201606L
| #include <thread> | ||
| #endif | ||
|
|
||
| // TODO: str_raw_ptr is not two-phase name lookup clean (https://github.com/Microsoft/wil/issues/8) |
There was a problem hiding this comment.
It's been too long since I've last thought about this issue, but I wonder - if we're introducing a new function, is there's something we can do to fix this for view_from_string. Reading my past comments in #8, it looks like there's a dependency in existing code on implicit conversions, namely T -> PCWSTR and T -> std::wstring, which makes things tricky... I'll have to noodle on this a bit more, but I'm curious if we can switch to a class template1 and have the base implementation assume that T is convertible to PCWSTR. This leaves out types that are implicitly convertible to std::wstring, but hopefully there are few enough instances that we can just fix them with static_cast (or more ideally, function calls). My comments in #8 seem to suggest that the main reliance on this conversion was with std::filesystem::path, in which case explicitly calling .native() is probably more ideal anyway since it returns by reference. That said, this is an experiment we'd probably have to carry out in the OS repo to gain better confidence.
[1] Function templates don't allow partial specialization and are therefore not suitable here.
There was a problem hiding this comment.
Side note: we can also maybe provide a specialization for std::filesystem::path in stl.h. The issue there is that due to annoying reasons there's no knowing which <filesystem> we'll be picking up. My hope here is that maybe feature test macros could be used, but I haven't dug into it.
Another possibility is to have detection logic in the base class template for something that is "string view like." E.g. something that has .data() and .size() members, though we need to be careful there - std::array has both .data() and .size() members, so we'd want other checks, for example checking for .length() (perhaps instead of .size()), though I'm not sure if that matches other types we don't want to match against.
There was a problem hiding this comment.
Although it seems my memory is wrong... std::filesystem::path doesn't have .data()/.size() like I thought... It does have .c_str() - maybe we could detect that, although that's wasteful since we know the length... Otherwise, it does have .native() but that's too generic of a name to make assumptions about...
| } | ||
|
|
||
| // NOTE: 'Strings' must all be PCWSTR, or convertible to PCWSTR, but C++ doesn't allow us to express that cleanly | ||
| // NOTE: 'Strings' must all be string_view_t, or convertible to string_view_t, but C++ doesn't allow us to express that cleanly |
There was a problem hiding this comment.
| // NOTE: 'Strings' must all be string_view_t, or convertible to string_view_t, but C++ doesn't allow us to express that cleanly | |
| // NOTE: 'Strings' must all be string_view_t, but C++ doesn't allow us to express that cleanly |
Second part's not really true anymore
There was a problem hiding this comment.
Actually... second part wasn't even true anymore before this
2 participants
This fixes #505
String views are not (necessarily) null terminated. Previously,
wil::str_concatrequired only "things that can bewchar_t const*converted." This change expands that to support string views. Note that there is nowistd::pair<>- one is declared inwistd_type_traits.hbut no body is provided.