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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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:
> 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..
Right, I meant
```
return _LIBCPP_ASSERT(__s != nullptr, "null pointer passed to non-null argument of char_traits<...>::length"), void(), _Traits::length(__s);
```
@ldionne's reply indicates that it's not possible to regression-test this, because the type of `_LIBCPP_ASSERT(...)` already happens to be `void`. (So the `, void()` does nothing. But I still think it might be a good idea, so the maintainer doesn't //have// to go look up the type of `_LIBCPP_ASSERT`.)

> I thought we poisoned `operator,` in the test suite.

The test iterators do poison it for themselves alone. But (at the moment) it's not possible for us to provide a completely generic top-level hijacker `template<class T, class U> void operator,(T, U);` because some of the //test files// use the built-in `operator,` in for-loops and stuff. We'd first have to remove all `for (...; ++i, ++j)` from the test files, and then we could turn on a generic `operator,` if we wanted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121231