This is an archive of the discontinued LLVM Phabricator instance.

Differential D121407

Allow temporary string_view((const char*)nullptr) while cleanup occurs.
AbandonedPublic

Authored by EricWF on Mar 10 2022, 1:12 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

Google is working to clean up a local patch to libc++ that allows
string_view to be constructed from null pointers. In order to do this we
need to intercept the call to char_traits::length. This temporary change
allows google to live at head with libc++ while it cleans up
non-conforming code.

Diff Detail

Unit TestsFailed

TimeTest
1,640 mslibcxx CI C++2b > llvm-libc++-shared-cfg-in.libcxx/debug::db_string_view.pass.cpp
1,800 mslibcxx CI GCC 11 / C++latest > llvm-libc++-shared-gcc-cfg-in.libcxx/debug::db_string_view.pass.cpp
550 mslibcxx CI Modular build > llvm-libc++-shared-cfg-in.libcxx/debug::db_string_view.pass.cpp

Event Timeline

EricWF created this revision.Mar 10 2022, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 1:12 PM
EricWF requested review of this revision.Mar 10 2022, 1:12 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 10 2022, 1:12 PM
Herald added a subscriber: libcxx-commits. · View Herald Transcript
EricWF updated this revision to Diff 414479.Mar 10 2022, 1:15 PM
  • Make code compile (Spell the parameter name correctly)
Harbormaster completed remote builds in B153635: Diff 414479.Mar 10 2022, 1:36 PM
ldionne added a comment.Mar 11 2022, 5:55 AM

I'll go ahead and land D121231, and we can land this patch on top if it makes your life easier in terms of upstream/downstream maintenance.

libcxx/include/string_view
320

With this patch, we are not asserting that __s != nullptr. I'd be OK with the patch if that were addressed. TBH I'm mostly neutral on how we solve this problem, as long as we get rid of the naked call to __libcpp_debug_function and we retain the _LIBCPP_ASSERT(__s != nullptr) upstream.

ldionne added a comment.Mar 16 2022, 1:48 PM

I landed my patch last week -- I assume you resolved the merge conflict internally already. If we want to move forward with something like this, let's do it, otherwise, we can abandon the patch to clear up the review queue!

EricWF abandoned this revision.Mar 16 2022, 4:08 PM

Revision Contents

PathSize
libcxx/
include/
7 lines
11 lines
2 lines

Diff 414479

libcxx/include/__config

Context not available.
# define _LIBCPP_PACKED_BYTE_FOR_AIX_END /* empty */ # define _LIBCPP_PACKED_BYTE_FOR_AIX_END /* empty */
#endif #endif
// Used to allow Google to transition to std::string_view while cleaning up
// code that constructs it from `nullptr` -- which is undefined.
// TODO(EricWF) Remove me!
#ifndef _LIBCPP_CHAR_TRAITS_LENGTH_HOOK
# define _LIBCPP_CHAR_TRAITS_LENGTH_HOOK(_TraitsType, __str) _TraitsType::length(__str)
#endif
#endif // __cplusplus #endif // __cplusplus
#endif // _LIBCPP_CONFIG #endif // _LIBCPP_CONFIG
Context not available.

libcxx/include/__string

Context not available.
} }
#endif // _LIBCPP_HAS_NO_WIDE_CHARACTERS #endif // _LIBCPP_HAS_NO_WIDE_CHARACTERS
template <class _Traits>
_LIBCPP_INLINE_VISIBILITY
_LIBCPP_CONSTEXPR
inline size_t __char_traits_length_checked(const typename _Traits::char_type* __s) _NOEXCEPT {
#if _LIBCPP_DEBUG_LEVEL >= 1
return __s ? _Traits::length(__s) : (_VSTD::__libcpp_debug_function(_VSTD::__libcpp_debug_info(__FILE__, __LINE__, "p == nullptr", "null pointer pass to non-null argument of char_traits<...>::length")), 0);
#else
return _Traits::length(__s);
#endif
}
#ifndef _LIBCPP_HAS_NO_CHAR8_T #ifndef _LIBCPP_HAS_NO_CHAR8_T
template <> template <>
Context not available.

libcxx/include/string_view

Context not available.
_LIBCPP_CONSTEXPR _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR _LIBCPP_INLINE_VISIBILITY
basic_string_view(const _CharT* __s) basic_string_view(const _CharT* __s)
: __data(__s), __size(_VSTD::__char_traits_length_checked<_Traits>(__s)) {} : __data(__s), __size(_LIBCPP_CHAR_TRAITS_LENGTH_HOOK(_Traits, __s)) {}
ldionneUnsubmitted
Not Done
ReplyInline Actions

With this patch, we are not asserting that __s != nullptr. I'd be OK with the patch if that were addressed. TBH I'm mostly neutral on how we solve this problem, as long as we get rid of the naked call to __libcpp_debug_function and we retain the _LIBCPP_ASSERT(__s != nullptr) upstream.

ldionne: With this patch, we are not asserting that `__s != nullptr`. I'd be OK with the patch if that…
#if _LIBCPP_STD_VER > 20 #if _LIBCPP_STD_VER > 20
basic_string_view(nullptr_t) = delete; basic_string_view(nullptr_t) = delete;
Context not available.