[libcxx-commits] [PATCH] D129823: [libc++][ranges] Make range algorithms support proxy iterators

Tue Jul 26 13:57:42 PDT 2022

rupprecht added a comment.

In D129823#3680525 <https://reviews.llvm.org/D129823#3680525>, @ldionne wrote:

> Thanks everyone for chiming in on this issue. I am speaking to @var-const right now who summarized the issues to me. It looks like there is some UB in MySQL right now because their funky iterator type is lying about `iterator_traits<It>::reference`, and that's really the crux of the problem IMO. That needs to be fixed, and it's not something that libc++ can accommodate by making the code work.
>
> **However**, it also seems like it's something that we can easily catch at compile-time instead, hence my suggestion <https://reviews.llvm.org/D130538#inline-1256036> to add a `static_assert` in D130538 <https://reviews.llvm.org/D130538>. It will still break Chromium via its MySQL dependency, however the error will clearly point out how it is broken and how to fix it.

Emphatic +1 to this, fixing the UB is better than hacking up libc++, and a static assert would be amazing here. I'd much rather handle a clear build breakage than spend hours/days hunting down the source of a weird runtime error.

> @rupprecht If you are able to get us a reproduction of the failure on top of D130538 <https://reviews.llvm.org/D130538> (without the `static_assert`), we'd be interested in taking a look to understand whether there's something more subtle at play. However, our current understanding is that D130538 <https://reviews.llvm.org/D130538> + `static_assert` will work in all cases (i.e. either it will work as intended, or it will issue a compile-time diagnostic). If you see additional issues after we move forward with D130538 <https://reviews.llvm.org/D130538>, please let us know via the usual means. This has been really useful in finding subtle problems early and we definitely want to address such issues with the highest priority.

I am attempting to do this, although it's a little more challenging now that the failure is moved later -- it was relatively easy to provide the previous repro because the test runner crashed at the moment something went wrong; producing wrong results without crashing requires more specific knowledge of mysql internals.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129823