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

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Thu Mar 10 13:48:59 PST 2022


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:
> Quuxplusone wrote:
> > 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.
> Or we could put the `operator,` overload in namespace std. Then we only have to fix cases where ADL would kick in within the tests.
> 
> I could have sworn I did that previously. Like it lived in `nasty_macro.h`.
> Then we only have to fix cases where ADL would kick in

Apparently a catch-all `operator,(T,U)` still doesn't affect when both sides are `int`; but it turns out that a lot of those `++i, ++j` have `i` as some container iterator type.

After digging into that, I've got some local diffs inserting `, void(), ` in test code. I might turn that into a PR in a day or two if nobody beats me to it.

> I could have sworn I did that previously.

(FYI, `nasty_macro.h` is now `libcxx/test/libcxx/nasty_macros.compile.pass.cpp`.) Nope.  I mean, it does ring a bell vaguely for me too, but I see no trace in the codebase, so I assume I must be thinking of the test iterators. `struct nasty_mutex` does overload its own `operator,`, too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121231



More information about the libcxx-commits mailing list