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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added reviewers: var-const, Mordante.
Quuxplusone added a comment.

In D119633#3323305 <https://reviews.llvm.org/D119633#3323305>, @philnik wrote:

> In D119633#3317770 <https://reviews.llvm.org/D119633#3317770>, @Quuxplusone wrote:
>
>> 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.

Yeah, simplifying might not be possible, and on closer inspection is at least not low-hanging fruit. Microsoft's `insert` is about as complicated these days: https://github.com/microsoft/STL/blob/8096a27/stl/inc/xstring#L3437-L3455

And `__addr_in_range` is already non-constexpr; let's keep it that way. My comment was based on your commit message, which said "__addr_in_range cannot be known during constant evaluation, so it has to always return true during constant evaluation" — that's incorrect. `__addr_in_range` is simply a non-constexpr function; it doesn't return //anything// during constant evaluation because it can't be called. (And this is as it should be.) Please update the commit message to say something like "`__addr_in_range` is a non-constexpr function, so we shouldn't call it during constant evaluation" or something like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119633