[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 inline comments.


================
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:
> 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?
> That makes the type of the expression `void`, no?

To answer my own question: Only if it's done to the last operand.

I thought we poisoned `operator,` in the test suite. If we still do, we should ensure there is a test that would have caught the uncasted operator, usage..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121231