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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

In D121231#3372632 <https://reviews.llvm.org/D121231#3372632>, @ldionne wrote:

> 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);
+}
----------------
Quuxplusone wrote:
> ldionne wrote:
> > 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.
> I'm not thrilled by `return _LIBCPP_ASSERT...` either, but I do agree with Louis that I'd rather see `_LIBCPP_ASSERT` than the raw call to `__libcpp_debug_function`.
> To fix the `operator,` issue, it's cheap and easy to replace the comma with `, void(), `; I think we should do that.
That makes the type of the expression `void`, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121231