[libcxx-commits] [PATCH] D119633: [libc++] Remove recursion in basic_string::insert(const_iterator, ForwardIterator, ForwardIterator)

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D119633#3317770 <https://reviews.llvm.org/D119633#3317770>, @Quuxplusone wrote:

> I think this code looks correct enough, but it would be a lot less scary if you could just make `__addr_in_range` a non-constexpr function, instead of having it "lie" during constexpr evaluation and get us into infinite loops. E.g.:
>
>   if (__libcpp_is_constant_evaluated() || !__string_is_trivial_iterator<_ForwardIterator>::value || __addr_in_range(*__first)) {
>       // Make a copy and then insert from the copy.
>       const basic_string __temp(__first, __last, __alloc());
>       return __insert_from_safe_copy(__n, __ip, __temp.begin(), __temp.end());
>   } else {
>       return __insert_from_safe_copy(__n, __ip, __first, __last);
>   }
>
> Note that `__insert_from_safe_copy` might be a better name than `__insert_not_in_range`, because being overlapping is just //one// of the many failure modes we need to guard against here.
>
> Read (or re-read) https://quuxplusone.github.io/blog/2021/04/17/pathological-string-appends/ before deciding on this approach. "Burn the whole thing to the ground and rewrite it" //might// be a reasonable alternative, especially if the alternative is //increasing// the complexity of this already-complex code.

What would you like to see changed, other than the name? I don't really see a way to make this code simpler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119633