Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
190 changes: 165 additions & 25 deletions include/wil/stl.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
#ifndef WI_STL_FAIL_FAST_IF
#define WI_STL_FAIL_FAST_IF FAIL_FAST_IF
#endif
#ifndef WI_STL_FAIL_FAST_IF_NULL
#define WI_STL_FAIL_FAST_IF_NULL FAIL_FAST_IF_NULL
#endif
Comment on lines +30 to +32

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note that the other definition was to work around a conflict with FAIL_FAST_IF. AFAIK such a conflict doesn't exist for FAIL_FAST_IF_NULL. That said, if you take my suggestion, this define isn't needed anyway

/// @endcond

#if defined(WIL_ENABLE_EXCEPTIONS)
Expand Down Expand Up @@ -160,6 +163,55 @@ inline wil::unique_bstr make_bstr(std::wstring_view source)
#endif // WIL_ENABLE_EXCEPTIONS
#endif // defined(__WIL_OLEAUTO_H_)

template <class TChar, class Traits = std::char_traits<TChar>>
class basic_zstring_view;

/**
Traits policy for a basic_zstring_view whose constructors reject null pointers and whose default constructor points
at an internal empty string. The nested char_traits alias keeps the resulting basic_zstring_view derived from the
same std::basic_string_view specialization as the nullable form.

@note basic_zstring_view publicly inherits from std::basic_string_view. A caller can explicitly cast to a mutable
base reference and assign a view with null data, bypassing the policy. Avoid mutating the object through a base
Comment on lines +174 to +175

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The issue is more general than this and is not specific to the nonnull type. E.g. you can assign non-null terminated data to both zstring_view and nonnull_zstring_view in this manner

reference.
*/
template <typename TChar, typename Traits = std::char_traits<TChar>>
struct nonnull_zstring_view_traits
{
using char_traits = Traits;
static constexpr bool empty_strings_are_non_null = true;
};

namespace details

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Missing /// @cond and /// @endcond pair

{
template <typename TChar, typename Traits, typename = void>
struct zstring_view_traits
{
static constexpr bool empty_strings_are_non_null = false;
using char_traits = Traits;
};

template <typename TChar, typename Traits>
struct zstring_view_traits<TChar, Traits, std::void_t<decltype(Traits::empty_strings_are_non_null)>>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
struct zstring_view_traits<TChar, Traits, std::void_t<decltype(Traits::empty_strings_are_non_null)>>
struct zstring_view_traits<TChar, Traits, std::void_t<decltype(Traits::empty_strings_are_non_null), typename Traits::char_traits>>

Otherwise this would fail if not provided.

{
static constexpr bool empty_strings_are_non_null = Traits::empty_strings_are_non_null;
using char_traits = typename Traits::char_traits;
};

template <typename T>
struct is_basic_zstring_view : std::false_type
{
};

template <typename TChar, typename Traits>
struct is_basic_zstring_view<basic_zstring_view<TChar, Traits>> : std::true_type
{
};

template <typename TChar>
inline constexpr TChar zstring_view_empty_storage[1]{TChar()};
} // namespace details

/**
zstring_view. A zstring_view is identical to a std::string_view except it is always nul-terminated (unless empty).
* zstring_view can be used for storing string literals without "forgetting" the length or that it is nul-terminated.
Expand All @@ -169,11 +221,18 @@ inline wil::unique_bstr make_bstr(std::wstring_view source)
* substr(pos) returns a zstring_view because the tail remains nul-terminated. substr(pos, count) returns a
std::string_view because an arbitrary slice may not be nul-terminated.
* contains() is available before C++23 through a compatibility implementation.
* nonnull_zstring_view uses a traits policy so its constructors produce non-null data(), including after default
construction.
*/
template <class TChar>
class basic_zstring_view : public std::basic_string_view<TChar>
template <class TChar, class Traits>
class basic_zstring_view : public std::basic_string_view<TChar, typename details::zstring_view_traits<TChar, Traits>::char_traits>
{
using size_type = typename std::basic_string_view<TChar>::size_type;
using ZStringViewTraits = details::zstring_view_traits<TChar, Traits>;
using BaseType = std::basic_string_view<TChar, typename ZStringViewTraits::char_traits>;
using size_type = typename BaseType::size_type;

template <class, class>
friend class basic_zstring_view;

template <typename T>
struct has_c_str
Expand All @@ -196,47 +255,94 @@ class basic_zstring_view : public std::basic_string_view<TChar>
};

public:
constexpr basic_zstring_view() noexcept = default;
constexpr basic_zstring_view() noexcept : BaseType(default_view())
{
}
constexpr basic_zstring_view(const basic_zstring_view&) noexcept = default;
constexpr basic_zstring_view& operator=(const basic_zstring_view&) noexcept = default;

constexpr basic_zstring_view(const TChar* pStringData, size_type stringLength) noexcept :
std::basic_string_view<TChar>(pStringData, stringLength)
BaseType(require_non_null(pStringData), stringLength)
{
if (pStringData[stringLength] != 0)
if constexpr (ZStringViewTraits::empty_strings_are_non_null)
{
// The test harness records fail-fast and returns, so do not dereference a rejected null pointer afterward.
if ((pStringData != nullptr) && (pStringData[stringLength] != 0))
{
WI_STL_FAIL_FAST_IF(true);
}
}
else if (pStringData[stringLength] != 0)
{
WI_STL_FAIL_FAST_IF(true);
}
Comment on lines +267 to 278

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you take my suggestion from down below, this all simplifies to a single call to check<true>()

}

template <size_t stringArrayLength>
constexpr basic_zstring_view(const TChar (&stringArray)[stringArrayLength]) noexcept :
std::basic_string_view<TChar>(&stringArray[0], length_n(&stringArray[0], stringArrayLength))
BaseType(&stringArray[0], length_n(&stringArray[0], stringArrayLength))
{
}

template <typename T = Traits, std::enable_if_t<details::zstring_view_traits<TChar, T>::empty_strings_are_non_null, int> = 0>
basic_zstring_view(std::nullptr_t) = delete;
Comment on lines +287 to +288

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The SFINAE is probably unnecessary here if I'm understanding things correctly. For zstring_view, construction with nullptr will forward to the TChar* constructor, which is UB for null pointers, so any existing callers are guaranteed to be wrong. It's worth noting that the nullptr_t constructor is deleted starting in C++23 as well. My vote is to unconditionally delete this and keep default construction as the only (reasonable) way to get a null pointer.


// Construct from nul-terminated char ptr. To prevent this from overshadowing array construction,
// we disable this constructor if the value is an array (including string literal).
template <typename TPtr, std::enable_if_t<std::is_convertible<TPtr, const TChar*>::value && !std::is_array<TPtr>::value>* = nullptr>
constexpr basic_zstring_view(TPtr&& pStr) noexcept : std::basic_string_view<TChar>(std::forward<TPtr>(pStr))
constexpr basic_zstring_view(TPtr&& pStr) noexcept : BaseType(require_non_null(std::forward<TPtr>(pStr)))
{
}

constexpr basic_zstring_view(const std::basic_string<TChar>& str) noexcept : BaseType(&str[0], str.size())
{
}

constexpr basic_zstring_view(const std::basic_string<TChar>& str) noexcept :
std::basic_string_view<TChar>(&str[0], str.size())
template <
typename TSrc,
std::enable_if_t<
has_c_str<TSrc>::value && has_size<TSrc>::value && std::is_same_v<typename TSrc::value_type, TChar> &&
!details::is_basic_zstring_view<std::decay_t<TSrc>>::value>* = nullptr>
constexpr basic_zstring_view(TSrc const& src) noexcept : BaseType(require_non_null(src.c_str()), src.size())
{
}

template <typename TSrc, std::enable_if_t<has_c_str<TSrc>::value && has_size<TSrc>::value && std::is_same_v<typename TSrc::value_type, TChar>>* = nullptr>
constexpr basic_zstring_view(TSrc const& src) noexcept : std::basic_string_view<TChar>(src.c_str(), src.size())
template <
typename TSrc,
std::enable_if_t<
has_c_str<TSrc>::value && !has_size<TSrc>::value && std::is_same_v<typename TSrc::value_type, TChar> &&
!details::is_basic_zstring_view<std::decay_t<TSrc>>::value>* = nullptr>
constexpr basic_zstring_view(TSrc const& src) noexcept : BaseType(require_non_null(src.c_str()))
{
}

template <typename TSrc, std::enable_if_t<has_c_str<TSrc>::value && !has_size<TSrc>::value && std::is_same_v<typename TSrc::value_type, TChar>>* = nullptr>
constexpr basic_zstring_view(TSrc const& src) noexcept : std::basic_string_view<TChar>(src.c_str())
template <
typename OtherTraits,
std::enable_if_t<
!std::is_same_v<Traits, OtherTraits> && std::is_same_v<BaseType, typename basic_zstring_view<TChar, OtherTraits>::BaseType> &&
(!ZStringViewTraits::empty_strings_are_non_null || details::zstring_view_traits<TChar, OtherTraits>::empty_strings_are_non_null),
int> = 0>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why so many different types used with enable_if? Should just be consistent with what was there before with * = nullptr

constexpr basic_zstring_view(const basic_zstring_view<TChar, OtherTraits>& other) noexcept :

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You've added these "converting constructors" but did not do the same for the assignment operator. Consider if that should also be covered.

BaseType(other.data(), other.size())
{
}

template <
typename OtherTraits,
std::enable_if_t<
!std::is_same_v<Traits, OtherTraits> && std::is_same_v<BaseType, typename basic_zstring_view<TChar, OtherTraits>::BaseType> &&
ZStringViewTraits::empty_strings_are_non_null && !details::zstring_view_traits<TChar, OtherTraits>::empty_strings_are_non_null,
long> = 0>
explicit constexpr basic_zstring_view(const basic_zstring_view<TChar, OtherTraits>& other) noexcept :
BaseType(require_non_null(other.data()), other.size())
{
}

template <
typename OtherTraits,
std::enable_if_t<!std::is_same_v<Traits, OtherTraits> && !std::is_same_v<BaseType, typename basic_zstring_view<TChar, OtherTraits>::BaseType>, short> = 0>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
std::enable_if_t<!std::is_same_v<Traits, OtherTraits> && !std::is_same_v<BaseType, typename basic_zstring_view<TChar, OtherTraits>::BaseType>, short> = 0>
std::enable_if_t<!std::is_same_v<BaseType, typename basic_zstring_view<TChar, OtherTraits>::BaseType>, short> = 0>

The base types being different would imply that the traits types are different.

basic_zstring_view(const basic_zstring_view<TChar, OtherTraits>&) = delete;

// basic_string_view [] precondition won't let us read view[view.size()]; so we define our own.
WI_NODISCARD constexpr const TChar& operator[](size_type idx) const noexcept
{
Expand All @@ -246,14 +352,18 @@ class basic_zstring_view : public std::basic_string_view<TChar>

WI_NODISCARD constexpr const TChar* c_str() const noexcept
{
if constexpr (ZStringViewTraits::empty_strings_are_non_null)
{
WI_ASSERT(this->data() != nullptr);
}
Comment on lines +355 to +358

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if constexpr (ZStringViewTraits::empty_strings_are_non_null)
{
WI_ASSERT(this->data() != nullptr);
}
WI_ASSERT(!ZStringViewTraits::empty_strings_are_non_null || (this->data() != nullptr));

Unless this triggers a bunch of "conditional expression is constant" warnings, I'd say to optimize for lines of code for debug-only statements.

WI_ASSERT(this->data() == nullptr || this->data()[this->size()] == 0);
return this->data();
}

// contains() backport for builds below C++23. Compiles out once the STL provides
// basic_string_view::contains natively.
#if !defined(__cpp_lib_string_contains) || __cpp_lib_string_contains < 202011L
WI_NODISCARD constexpr bool contains(std::basic_string_view<TChar> view) const noexcept
WI_NODISCARD constexpr bool contains(BaseType view) const noexcept
{
return this->find(view) != this->npos;
}
Expand All @@ -271,20 +381,45 @@ class basic_zstring_view : public std::basic_string_view<TChar>

WI_NODISCARD constexpr basic_zstring_view substr(size_type pos = 0) const
{
const auto tail = std::basic_string_view<TChar>(*this).substr(pos);
const auto tail = BaseType(*this).substr(pos);
return tail.data() == nullptr ? basic_zstring_view{} : basic_zstring_view{tail.data(), tail.size()};
}

WI_NODISCARD constexpr std::basic_string_view<TChar> substr(size_type pos, size_type count) const
WI_NODISCARD constexpr BaseType substr(size_type pos, size_type count) const
{
return std::basic_string_view<TChar>(*this).substr(pos, count);
return BaseType(*this).substr(pos, count);
}

private:
static constexpr BaseType default_view() noexcept
{
if constexpr (ZStringViewTraits::empty_strings_are_non_null)
{
return BaseType(&details::zstring_view_empty_storage<TChar>[0], 0);
}
else
{
return BaseType{};
}
}

static constexpr const TChar* require_non_null(const TChar* value) noexcept
{
if constexpr (ZStringViewTraits::empty_strings_are_non_null)
{
value = WI_STL_FAIL_FAST_IF_NULL(value);
if (value == nullptr)
{
return &details::zstring_view_empty_storage<TChar>[0];
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fairly certain I know what this is trying to do and why, however someone less familiar with how the tests are structured and work could easily look at this and think there's a mistake or something of that nature, so I'd like to try and reduce the complexity here, which I believe should be possible. The best suggestion I have at the moment is to change this to something more like:

template <bool CheckTerminator>
void check()
{
    [[maybe_unused]] auto ptr = this->data();
    [[maybe_unused]] auto len = this->size();
    if constexpr(CheckTerminator && ZStringViewTraits::empty_strings_are_non_null)
    {
        WI_STL_FAIL_FAST_IF(!ptr || (ptr[len] != 0));
    }
    else if constexpr (ZStringViewTraits::empty_strings_are_non_null)
    {
        WI_STL_FAIL_FAST_IF(!ptr);
    }
    else if constexpr (CheckTerminator)
    {
        WI_STL_FAIL_FAST_IF(ptr[len] != 0);
    }
}

Effectively, this combines the two checks - null and null terminated - into a single fail-fast check. That is, you wouldn't have the issue where a "fail-fast" would get issued, recorded in the test, and then continue execution only to crash on a null pointer read. You could then modify the constructors as follows (require_non_null is assumed to no longer exist):

  • Default constructor: no change needed
  • Copy constructor/assignment operator: no change needed
  • Pointer+length constructor: call check<true>() in the body
  • Array constructor: no change needed
  • nullptr_t constructor: delete unconditionally; see the other comment
  • Convertible to const TChar* constructor: call check<false>() in the body
  • basic_string constructor: no change needed
  • "String-like" (has c_str and size) constructor: call check<false>() in the body
  • "Path-like" (has c_str but no size) constructor: call check<false>() in the body
  • Non-explicit conversion constructor: no change needed
  • explicit conversion constructor: call check<false>() in the body
  • Deleted conversion constructor: no change needed

}
return value;
}

// Bounds-checked version of char_traits::length, like strnlen. Requires that the input contains a null terminator.
static constexpr size_type length_n(_In_reads_opt_(buf_size) const TChar* str, size_type buf_size) noexcept
{
const std::basic_string_view<TChar> view(str, buf_size);
const BaseType view(str, buf_size);
auto pos = view.find_first_of(TChar());
if (pos == view.npos)
{
Expand All @@ -294,17 +429,21 @@ class basic_zstring_view : public std::basic_string_view<TChar>
}

// The following basic_string_view methods must not be allowed because they break the nul-termination.
using std::basic_string_view<TChar>::swap;
using std::basic_string_view<TChar>::remove_suffix;
using BaseType::remove_suffix;
using BaseType::swap;
};

using zstring_view = basic_zstring_view<char>;
using zwstring_view = basic_zstring_view<wchar_t>;

// Variants that reject null construction and default to a non-null empty string.
using nonnull_zstring_view = basic_zstring_view<char, nonnull_zstring_view_traits<char>>;
using nonnull_zwstring_view = basic_zstring_view<wchar_t, nonnull_zstring_view_traits<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)
template <typename TChar, typename Traits>
inline auto str_raw_ptr(basic_zstring_view<TChar, Traits> str)
{
return str.c_str();
}
Expand Down Expand Up @@ -423,8 +562,9 @@ overloaded(T...) -> overloaded<T...>;
#ifndef WIL_SUPPRESS_STD_FORMAT_USE
#if (__WI_LIBCPP_STD_VER >= 20) && WI_HAS_INCLUDE(<format>, 1) // Assume present if C++20
#include <format>
template <typename TChar>
struct std::formatter<wil::basic_zstring_view<TChar>, TChar> : std::formatter<std::basic_string_view<TChar>, TChar>
template <typename TChar, typename Traits>
struct std::formatter<wil::basic_zstring_view<TChar, Traits>, TChar>
: std::formatter<std::basic_string_view<TChar, typename wil::details::zstring_view_traits<TChar, Traits>::char_traits>, TChar>
{
};
#endif
Expand Down
77 changes: 77 additions & 0 deletions tests/StlTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,20 @@ TEST_CASE("StlTests::TestZStringView formatting", "[stl][zstring_view]")
auto fmtStr = std::format("Hello {}", str);
REQUIRE(fmtStr == "Hello kittens");
}

SECTION("nonnull_zstring_view can be used with std::format")
{
wil::nonnull_zstring_view str{"kittens"};
auto fmtStr = std::format("Hello {}", str);
REQUIRE(fmtStr == "Hello kittens");
}

SECTION("nonnull_zwstring_view can be used with std::format")
{
wil::nonnull_zwstring_view str{L"kittens"};
auto fmtStr = std::format(L"Hello {}", str);
REQUIRE(fmtStr == L"Hello kittens");
}
}

#endif
Expand Down Expand Up @@ -326,4 +340,67 @@ TEST_CASE("StlTests::TestZStringView substr and contains", "[stl][zstring_view]"
test(wil::zwstring_view{L"Hello, World!"}, wil::zwstring_view{L"World!"}, L"Hello", L"missing", L'W', L'x');
}

TEST_CASE("StlTests::TestNonNullZStringView", "[stl][zstring_view][nonnull]")
{
const auto test = [](auto nonnullDefault, auto nullableDefault, auto text) {
using nonnull_type = decltype(nonnullDefault);
using nullable_type = decltype(nullableDefault);
using char_type = typename nonnull_type::value_type;
using string_view_type = std::basic_string_view<char_type>;

STATIC_REQUIRE(sizeof(nonnull_type) == sizeof(nullable_type));
STATIC_REQUIRE(std::is_trivially_copyable_v<nonnull_type>);
STATIC_REQUIRE(!std::is_constructible_v<nonnull_type, std::nullptr_t>);
STATIC_REQUIRE(std::is_convertible_v<nonnull_type, nullable_type>);
STATIC_REQUIRE(!std::is_convertible_v<nullable_type, nonnull_type>);
STATIC_REQUIRE(std::is_constructible_v<nonnull_type, nullable_type>);

REQUIRE(nullableDefault.data() == nullptr);
REQUIRE(nonnullDefault.data() != nullptr);
REQUIRE(nonnullDefault.empty());
REQUIRE(nonnullDefault.c_str()[0] == char_type{});

nonnull_type fromLiteral{text};
REQUIRE(fromLiteral.data() != nullptr);
REQUIRE(fromLiteral.c_str()[fromLiteral.size()] == char_type{});
REQUIRE(wil::str_raw_ptr(fromLiteral) == fromLiteral.c_str());

string_view_type& baseReference = fromLiteral;
REQUIRE(baseReference.data() == fromLiteral.data());
REQUIRE(baseReference.size() == fromLiteral.size());

nullable_type nullable = fromLiteral;
REQUIRE(nullable.data() == fromLiteral.data());
REQUIRE(nullable.size() == fromLiteral.size());

nonnull_type checked{nullable};
REQUIRE(checked.data() == nullable.data());
REQUIRE(checked.size() == nullable.size());

auto emptyTail = nonnullDefault.substr();
REQUIRE(emptyTail.data() != nullptr);
REQUIRE(emptyTail.empty());

const char_type* nullPointer = nullptr;
REQUIRE_ERROR((nonnull_type{nullPointer}));
REQUIRE_ERROR((nonnull_type{nullPointer, 0}));
REQUIRE_ERROR((nonnull_type{nullableDefault}));
};

test(wil::nonnull_zstring_view{}, wil::zstring_view{}, "hello");
test(wil::nonnull_zwstring_view{}, wil::zwstring_view{}, L"hello");

struct custom_char_traits : std::char_traits<char>
{
};
using custom_nonnull = wil::basic_zstring_view<char, wil::nonnull_zstring_view_traits<char, custom_char_traits>>;
using custom_nullable = wil::basic_zstring_view<char, custom_char_traits>;
using custom_base = std::basic_string_view<char, typename wil::details::zstring_view_traits<char, custom_char_traits>::char_traits>;
STATIC_REQUIRE(std::is_base_of_v<std::basic_string_view<char, custom_char_traits>, custom_nonnull>);
STATIC_REQUIRE(!std::is_same_v<std::string_view, custom_base>);
STATIC_REQUIRE(!std::is_constructible_v<wil::zstring_view, custom_nullable>);
STATIC_REQUIRE(!std::is_constructible_v<custom_nonnull, wil::zstring_view>);
STATIC_REQUIRE(!std::is_constructible_v<wil::nonnull_zstring_view, custom_nullable>);
}

#endif
Loading