Skip to content

Enable using string views with wil::str_concat #590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jonwis wants to merge 4 commits into
base: master
Choose a base branch
from
+104 −22
Open

Enable using string views with wil::str_concat #590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c0670e8
Enable combine/append of string view things
jonwis May 25, 2025
85a381a
Add a case
jonwis Dec 30, 2025
75727f9
Merge remote-tracking branch 'origin/master' into user/jonwis/string-…
jonwis Dec 30, 2025
704ffe4
Merge branch 'master' into user/jonwis/string-append-views
dunhor Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions CMakePresets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"installDir": "${sourceDir}/build/install/${presetName}",
"toolchainFile": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake",
"cacheVariables": {
"WIL_ENABLE_ASAN": true,
"CMAKE_CONFIGURATION_TYPES": "Debug;RelWithDebInfo;Release;MinSizeRel",
"CMAKE_CXX_COMPILER": "cl",
"CMAKE_C_COMPILER": "cl"
Expand All @@ -29,29 +30,33 @@
}
},
"cacheVariables": {
"WIL_ENABLE_ASAN": false,
"CMAKE_CXX_COMPILER": "clang-cl",
"CMAKE_C_COMPILER": "clang-cl"
}
},
{
"name": "clang-release",
"inherits": "clang",
"hidden": false,
"cacheVariables": {
"WIL_ENABLE_ASAN": true,
"WIL_ENABLE_UBSAN": true
}
}
],
"buildPresets": [
{
"name": "msvc-debug",
"displayName": "MSVC Debug",
"configurePreset": "msvc",
"configuration": "Debug",
"cacheVariables": {
"WIL_ENABLE_ASAN": true
}
"configuration": "Debug"
},
{
"name": "msvc-release",
"displayName": "MSVC Release (debuggable)",
"configurePreset": "msvc",
"configuration": "RelWithDebInfo",
"cacheVariables": {
"WIL_ENABLE_ASAN": true
}
"configuration": "RelWithDebInfo"
},
{
"name": "clang-debug",
Expand All @@ -62,12 +67,8 @@
{
"name": "clang-release",
"displayName": "clang Release (debuggable)",
"configurePreset": "clang",
"configuration": "RelWithDebInfo",
"cacheVariables": {
"WIL_ENABLE_ASAN": true,
"WIL_ENABLE_UBSAN": true
}
"configurePreset": "clang-release",
"configuration": "RelWithDebInfo"
}
],
"testPresets": [
Expand Down
51 changes: 43 additions & 8 deletions include/wil/resource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1836,6 +1836,28 @@ PCWSTR str_raw_ptr(const unique_any_t<T>& val)
return str_raw_ptr(val.get());
}

struct string_view_t
Copy link
Member

@dunhor dunhor Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intent of this being "public" so that third parties can overload without touching the details namespace?

Copy link
Member

@dunhor dunhor Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
wchar_t const* data;
size_t length;
};

inline string_view_t view_from_string(string_view_t const& s)
{
return s;
}

inline string_view_t view_from_string(PCWSTR str)
{
return string_view_t{str, str ? wcslen(str) : 0};
}

template <typename T>
inline string_view_t view_from_string(wil::unique_any_t<T> const& str)
{
return view_from_string(str.get());
}

#if !defined(__WIL_MIN_KERNEL) && !defined(WIL_KERNEL_MODE)
/// @cond
namespace details
Expand All @@ -1847,12 +1869,12 @@ namespace details
// Concatenate any number of strings together and store it in an automatically allocated string. If a string is present
// in the input buffer, it is overwritten.
template <typename string_type>
HRESULT str_build_nothrow(string_type& result, _In_reads_(strCount) PCWSTR* strList, size_t strCount)
HRESULT str_build_nothrow_(string_type& result, _In_reads_(strCount) string_view_t const* strList, size_t strCount)
{
size_t lengthRequiredWithoutNull{};
for (auto& string : make_range(strList, strCount))
{
lengthRequiredWithoutNull += string ? wcslen(string) : 0;
lengthRequiredWithoutNull += string.length;
}

details::string_maker<string_type> maker;
Expand All @@ -1862,22 +1884,23 @@ namespace details
auto bufferEnd = buffer + lengthRequiredWithoutNull + 1;
for (auto& string : make_range(strList, strCount))
{
if (string)
if (string.data)
{
RETURN_IF_FAILED(StringCchCopyExW(buffer, (bufferEnd - buffer), string, &buffer, nullptr, STRSAFE_IGNORE_NULLS));
RETURN_IF_FAILED(StringCchCopyNExW(
buffer, (bufferEnd - buffer), string.data, string.length, &buffer, nullptr, STRSAFE_IGNORE_NULLS));
}
}

result = maker.release();
return S_OK;
}

// 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
Copy link
Member

@dunhor dunhor Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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

Copy link
Member

@dunhor dunhor Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually... second part wasn't even true anymore before this

template <typename string_type, typename... Strings>
HRESULT str_build_nothrow(string_type& result, Strings... strings)
{
PCWSTR localStrings[] = {strings...};
return str_build_nothrow(result, localStrings, sizeof...(Strings));
string_view_t localStrings[] = {strings...};
return str_build_nothrow_<string_type>(result, localStrings, ARRAYSIZE(localStrings));
}
} // namespace details
/// @endcond
Expand All @@ -1888,7 +1911,7 @@ template <typename string_type, typename... strings>
HRESULT str_concat_nothrow(string_type& buffer, const strings&... str)
{
static_assert(sizeof...(str) > 0, "attempting to concatenate no strings");
return details::str_build_nothrow(buffer, details::string_maker<string_type>::get(buffer), str_raw_ptr(str)...);
return details::str_build_nothrow(buffer, view_from_string(details::string_maker<string_type>::get(buffer)), view_from_string(str)...);
}
#endif // !defined(__WIL_MIN_KERNEL) && !defined(WIL_KERNEL_MODE)

Expand Down Expand Up @@ -4929,6 +4952,18 @@ inline PCWSTR str_raw_ptr(const unique_hstring& str)
return str_raw_ptr(str.get());
}

inline string_view_t view_from_string(HSTRING str)
{
UINT32 length = 0;
PCWSTR rawBuffer = WindowsGetStringRawBuffer(str, &length);
return string_view_t{rawBuffer, length};
}

inline string_view_t view_from_string(const unique_hstring& str)
{
return view_from_string(str.get());
}

#endif // __WIL__WINSTRING_H_
#if (defined(__WIL__WINSTRING_H_) && !defined(__WIL__WINSTRING_H_STL) && defined(WIL_RESOURCE_STL)) || defined(WIL_DOXYGEN)
/// @cond
Expand Down
23 changes: 23 additions & 0 deletions include/wil/stl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,16 @@ inline PCWSTR str_raw_ptr(const std::wstring& str)
return str.c_str();
}

inline string_view_t view_from_string(std::wstring_view const& s)
Copy link
Member

@dunhor dunhor Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should get moved down to be inside the #if __cpp_lib_string_view >= 201606L

{
return string_view_t{s.data(), s.length()};
}

inline string_view_t view_from_string(std::wstring const& s)
{
return string_view_t{s.data(), s.length()};
}

#if __cpp_lib_string_view >= 201606L
/**
zstring_view. A zstring_view is identical to a std::string_view except it is always nul-terminated (unless empty).
Expand Down Expand Up @@ -241,6 +251,19 @@ class basic_zstring_view : public std::basic_string_view<TChar>
using zstring_view = basic_zstring_view<char>;
using zwstring_view = basic_zstring_view<wchar_t>;

// str_raw_ptr is an overloaded function that retrieves a const pointer to the first character in a string's buffer.
// This is the overload for std::wstring. Other overloads available in resource.h.
template <typename TChar>
inline auto str_raw_ptr(basic_zstring_view<TChar> str)
{
return str.c_str();
}

inline string_view_t view_from_string(zwstring_view const& s)
{
return string_view_t{s.data(), s.length()};
}

inline namespace literals
{
constexpr zstring_view operator""_zv(const char* str, std::size_t len) noexcept
Expand Down
23 changes: 23 additions & 0 deletions tests/FileSystemTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,21 @@
// TODO: str_raw_ptr is not two-phase name lookup clean (https://github.com/Microsoft/wil/issues/8)
Copy link
Member

@dunhor dunhor Jan 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dunhor dunhor Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dunhor dunhor Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

namespace wil
{
struct string_view_t;

PCWSTR str_raw_ptr(HSTRING);
string_view_t view_from_string(HSTRING);
#ifdef WIL_ENABLE_EXCEPTIONS
PCWSTR str_raw_ptr(const std::wstring&);
string_view_t view_from_string(std::wstring const&);
string_view_t view_from_string(std::wstring_view const&);
#endif
} // namespace wil

#ifdef WIL_ENABLE_EXCEPTIONS
#include <wil/stl.h>
#endif

#include <wil/resource.h>
#include <wil/filesystem.h>

Expand Down Expand Up @@ -528,6 +537,20 @@ TEST_CASE("FileSystemTests::VerifyStrConcat", "[filesystem]")
REQUIRE_SUCCEEDED(wil::str_concat_nothrow(combinedStringNT, test2.c_str(), test3));
REQUIRE(CompareStringOrdinal(combinedStringNT.get(), -1, expectedStr, -1, TRUE) == CSTR_EQUAL);
}

#ifdef WIL_ENABLE_EXCEPTIONS
SECTION("Concat with views of things")
{
auto part1 = std::wstring_view(L"Test2");
auto part2 = std::wstring(L"Test3");
auto part3 = wil::zwstring_view(L"Test4");
auto part4 = wil::make_unique_string_nothrow<wil::unique_cotaskmem_string>(L"Test5");
auto part5 = wil::make_unique_string_nothrow<wil::unique_hstring>(L"Test6");
wil::unique_cotaskmem_string combinedStringNT = wil::make_unique_string_nothrow<wil::unique_cotaskmem_string>(L"Test1");
REQUIRE_SUCCEEDED(wil::str_concat_nothrow(combinedStringNT, part1, part2, part3, part4, part5));
REQUIRE(CompareStringOrdinal(combinedStringNT.get(), -1, L"Test1Test2Test3Test4Test5Test6", -1, TRUE) == CSTR_EQUAL);
}
#endif
}

TEST_CASE("FileSystemTests::VerifyStrPrintf", "[filesystem]")
Expand Down