[libcxx-commits] [PATCH] D121231: [libc++] Remove raw call to debug handler from __char_traits_length_checked

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D121231#3369852 <https://reviews.llvm.org/D121231#3369852>, @Quuxplusone wrote:

> In D121231#3368770 <https://reviews.llvm.org/D121231#3368770>, @EricWF wrote:
>
>> Please don't change this. Google uses this hook to change the behavior of string_view internally. We're working to get rid of the patch, but this helper function was specifically put in place to allow for the patch.
>
> @EricWF, sounds like Louis and I had two different interpretations of your request. :) Do you mean "Google wants the //name and signature// of `__char_traits_length_unchecked<_Traits>` to remain unchanged, please don't change it to `__assert_not_null`"? Or do you mean "Google wants the //implementation// of `__char_traits_length_unchecked<_Traits>` to remain unchanged, please don't change its body to use `_LIBCPP_ASSERT`"? I assumed you meant only the former; @ldionne's reply suggests that maybe you meant the latter. (IMVHO the latter is unreasonable, whereas the former is understandable but I agree Google should work with us to find a path forward and/or document what they need, because the current name-and-signature of `__char_traits_length_unchecked` is pretty weird.)

Agreed -- if only the name and signature must not change, we should be able to do this instead without breaking Google (a simple merge conflict resolution should be sufficient):

  template <class _Traits>
  _LIBCPP_INLINE_VISIBILITY
  _LIBCPP_CONSTEXPR
  inline size_t __char_traits_length_checked(const typename _Traits::char_type* __s) _NOEXCEPT {
    return _LIBCPP_ASSERT(__s != nullptr, "null pointer pass to non-null argument of char_traits<...>::length"), _Traits::length(__s);
  }

It doesn't change what I've said above -- we don't want to be "held hostage" to invisible downstream code, but this could definitely be a way to unblock the assertions work without making @EricWF 's life unnecessarily difficult. It would still be nice to understand what's the internal hook and whether there's a better way to properly support that in upstream.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121231/new/

https://reviews.llvm.org/D121231