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

[Hui via Phabricator via libcxx-commits]

huixie90 added a comment.

In D129823#3678367 <https://reviews.llvm.org/D129823#3678367>, @rupprecht wrote:

> In D129823#3678246 <https://reviews.llvm.org/D129823#3678246>, @var-const wrote:
>
>> @alexfh @jgorbe @rupprecht Thank you for the additional details! I believe the analysis by @huixie90 is spot on and have prepared a patch based on it: https://reviews.llvm.org/D130538
>> Can you try out the patch to see if it resolves the issue you're seeing? It does fix the repro case for me.
>
> Thanks! Unfortunately, it just changes the failure that we now see from a crash to a weird runtime failure; incorrectly sorted results, I assume.
>
> I have an alternative change to fix the UB and return `varlen_element &` instead of `varlen_element` (isn't that the eventual fix we should do anyway?), and initial tests seem to be passing now. I'm not sure what D130538 <https://reviews.llvm.org/D130538> is missing.

@rupprecht Thank you for trying the fix. You can't return `varlen_element&` because the function creates a temporary `varlen_element` on the fly. The fix is to remove the template specialization of `std::iterator_traits<varlen_iterator>` , which incorrectly defines the traits. instead , define some member typedef:

  using value_type = varlen_element;
  using reference = varlen_element;
  using difference_type = ptrdiff_t;
  using iterator_category = std::random_access_iterator_tag;
  using pointer = void; // this one is also wrong because of wrong operator->

note (pointer type and `operator->` also seem to be wrong but we don't use `->` in our algorithms.
https://godbolt.org/z/T8cxffb3o

Also the fact that `value_type` being the same as `reference` is a bit suspicious as most proxy iterators have different types. but it might be fine since the code worked before.

Could you please send us a reproducer where the runtime behaviour is wrong? It might be another UB somewhere else which exposed by this patch. We don't want to blindly break user code even if it is UB so it would be good to see a reproducer.
(Please could you send the expected output of the reproducer as well because I don't fully understand what the class is supposed to do atm)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129823