[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#3372617 <https://reviews.llvm.org/D121231#3372617>, @EricWF wrote:

> We would very much be open to upstreaming a version of change in order to reduce the burden to both the project and Google.  We are working diligently at the moment to remove the patch internally entirely. It's the primary focus of my work at the moment. So anything we upstream would be temporary on the order of 6 months to a year. Let me know what you think is appropriate here.

If we can upstream something like a temporary escape hatch to change the behavior of `std::string_view` so it accepts nullptr, I think it's reasonable, and we can remove it in the future once you've managed to migrate off of it. My understanding is that the diff is rather small and non viral.

> Otherwise, this changed looks fine to us. If you wouldn't mind giving me until this afternoon so I can negotiate landing the required changes at Google. The changes are simple but require some finesse to ensure they land at the right time

Yes, no worries. I'll ping you on Discord this afternoon to check if it's okay to land this.



================
Comment at: libcxx/include/string_view:244
+  // This needs to be a single statement for C++11 constexpr
+  return _LIBCPP_ASSERT(__s != nullptr, "null pointer passed to non-null argument of char_traits<...>::length"), _Traits::length(__s);
+}
----------------
EricWF wrote:
> If you use the ternary operator, you can make it a single statement without having to worry about custom char types that hijack ADL for operator,
> 
> Which is why it was written out that way initially.
> 
> Non built-in character types are allowed in string view, right?
You mean like `__s ? _Traits::length(__s) : _LIBCPP_ASSERT(false, "message")`? Yes, I think that would work. However, `_LIBCPP_ASSERT` is an expression casted to `(void)`, so I don't think it's possible to hijack `operator,` as currently written -- the statement expands roughly to `((void)assertion-expr), _Traits::length(__s)`.

> Non built-in character types are allowed in string view, right?

Yes, I definitely believe that's the case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121231