basic_zstring_view: add opt-in nonnull variants - #668
basic_zstring_view: add opt-in nonnull variants#668Monroe Thomas (mmthomas) wants to merge 6 commits into
Conversation
Add a traits policy that preserves the underlying char_traits type while enforcing non-null construction. Provide narrow and wide aliases, checked cross-variant conversion, and focused invariant, reference-conversion, custom-traits, formatting, and fail-fast tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5697cd0e-cf83-4d94-9f72-4b8c79379229
Detect a null pointer when c_str() is called after mutation through the public string_view base, and cover the inheritance escape hatch with a regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5697cd0e-cf83-4d94-9f72-4b8c79379229
Route nonnull pointer checks through FAIL_FAST_IF_NULL so diagnostics retain the checked expression and static analysis receives the pointer-specific contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5697cd0e-cf83-4d94-9f72-4b8c79379229
Gate cross-policy conversions on the exact string_view base type, reject incompatible specializations, and limit rebinding to explicitly marked policy traits. Use a debug assertion rather than a partial production fail-fast for base-class mutation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5697cd0e-cf83-4d94-9f72-4b8c79379229
Duncan Horn (dunhor)
left a comment
There was a problem hiding this comment.
I'm liking the way that this looks. My primary concern is around the non-intuitive complexity that I'm pretty sure exists for the tests. I offer a suggestion that I believe should both work with the tests and simplify the code. The other comments are more minor.
| @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 |
There was a problem hiding this comment.
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
| static constexpr bool empty_strings_are_non_null = true; | ||
| }; | ||
|
|
||
| namespace details |
There was a problem hiding this comment.
Missing /// @cond and /// @endcond pair
| }; | ||
|
|
||
| template <typename TChar, typename Traits> | ||
| struct zstring_view_traits<TChar, Traits, std::void_t<decltype(Traits::empty_strings_are_non_null)>> |
There was a problem hiding this comment.
| 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.
| 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; |
There was a problem hiding this comment.
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.
| 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> |
There was a problem hiding this comment.
Why so many different types used with enable_if? Should just be consistent with what was there before with * = nullptr
| if constexpr (ZStringViewTraits::empty_strings_are_non_null) | ||
| { | ||
| WI_ASSERT(this->data() != nullptr); | ||
| } |
There was a problem hiding this comment.
| 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.
| !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> | ||
| constexpr basic_zstring_view(const basic_zstring_view<TChar, OtherTraits>& other) noexcept : |
There was a problem hiding this comment.
You've added these "converting constructors" but did not do the same for the assignment operator. Consider if that should also be covered.
| if (value == nullptr) | ||
| { | ||
| return &details::zstring_view_empty_storage<TChar>[0]; | ||
| } |
There was a problem hiding this comment.
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_tconstructor: delete unconditionally; see the other comment- Convertible to
const TChar*constructor: callcheck<false>()in the body basic_stringconstructor: no change needed- "String-like" (has
c_strandsize) constructor: callcheck<false>()in the body - "Path-like" (has
c_strbut nosize) constructor: callcheck<false>()in the body - Non-
explicitconversion constructor: no change needed explicitconversion constructor: callcheck<false>()in the body- Deleted conversion constructor: no change needed
| 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); | ||
| } |
There was a problem hiding this comment.
If you take my suggestion from down below, this all simplifies to a single call to check<true>()
| #ifndef WI_STL_FAIL_FAST_IF_NULL | ||
| #define WI_STL_FAIL_FAST_IF_NULL FAIL_FAST_IF_NULL | ||
| #endif |
There was a problem hiding this comment.
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
Summary
wil::zstring_viewis a non-owning view of a null-terminated string. Its default constructor followsstd::string_view: the view is empty anddata()is null. That behavior must remain unchanged because existing callers may use null to mean "no string."Some callers instead need an empty view that can be passed directly to a C API without first checking for null. This PR adds opt-in
nonnull_zstring_viewandnonnull_zwstring_viewaliases for that use case. Their constructors reject null pointers, and their default constructors point at an internal empty string.The existing
zstring_viewandzwstring_viewaliases retain their current behavior.C++ standardization
WG21 is standardizing the same general abstraction for C++29 as
std::basic_cstring_viewin P3655R5,std::cstring_view. The proposed type is a non-owning view of a null-terminated string. Its default constructor refers to a static null terminator, sodata()andc_str()return a valid empty string rather than null, and directnullptrconstruction is deleted.P3655 is an active proposal rather than part of the published C++ standard. WIL retains its existing
zstring_viewnaming and public-inheritance design; this PR adds an opt-in construction policy that provides the proposal's non-null empty-state behavior without changing existing callers.Public API
The policy type keeps the non-null behavior separate from the character traits used by
std::basic_string_view. As a result,nonnull_zstring_viewandzstring_viewboth derive fromstd::string_view, rather than deriving from differentstd::basic_string_viewspecializations.This matters for normal C++ interoperability:
wil::nonnull_zstring_view value{"hello"}; std::string_view& base = value; // binds to the inherited base objectCustom character traits remain supported through
nonnull_zstring_view_traits<TChar, Traits>.Construction and conversion behavior
nullptrconstruction is deleted.std::basic_string, compatible string-like objects, and valid pointer inputs behave like the existing type.substr(pos)preserves the selected policy. A substring of a default-constructed non-null view therefore remains non-null.Inheritance limitation
basic_zstring_viewpublicly inherits fromstd::basic_string_view. This permits a caller to explicitly obtain a mutable base reference and assign a nullable base view:wil::nonnull_zstring_view value{"hello"}; std::string_view& base = value; base = std::string_view{}; // bypasses the non-null construction policyThe new type enforces non-null construction through its own API. In debug builds, the derived
c_str()asserts if base-class mutation has changed the stored pointer to null. Calls made directly through the base class still bypass that check. Removing the escape hatch entirely would require replacing the existing inheritance design rather than extending it.Compatibility
Existing code can continue to use
wil::zstring_viewandwil::zwstring_viewwith the same source syntax and nullable default behavior. Adding the defaultedTraitsparameter changes the compiler-generated linker name for functions that exposebasic_zstring_viewin a binary interface. Default construction also now runs the policy-selection constructor instead of being a trivial operation. Object layout, size, and trivial copyability remain unchanged.Tests
The focused tests cover both
charandwchar_tvariants:std::basic_string_viewbase-reference compatibility;substr(pos);Local validation:
witest.exe "[zstring_view]": 155 assertions passed.witest.cpplatest.exe "[zstring_view]": 159 assertions passed.Scope
This PR does not add new string literals, change SAL annotations, alter the existing nullable aliases, or redesign
basic_zstring_viewto remove public inheritance.The API direction originated in the compatibility and traits discussion on #635; this description is intended to stand on its own.