[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.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM; some remaining nits but they're all optional IIUC.



================
Comment at: libcxx/include/string:1439
+    template <class _ForwardIterator>
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17
+    iterator __insert_from_safe_copy(size_type __n, size_type __ip, _ForwardIterator __first, _ForwardIterator __last) {
----------------
Nit: I suggest following the guideline "Use the most aggressive constexpr possible for internal details," and I see no reason this couldn't be constexpr in C++14, right?


================
Comment at: libcxx/include/string:1465-1466
+
+    _LIBCPP_HIDE_FROM_ABI allocator_type& __alloc() _NOEXCEPT { return __r_.second(); }
+    _LIBCPP_HIDE_FROM_ABI const allocator_type& __alloc() const _NOEXCEPT { return __r_.second(); }
 
----------------
Personally I wouldn't have touched these lines. But I don't really care.


================
Comment at: libcxx/include/string:2818
+
+    if (__string_is_trivial_iterator<_ForwardIterator>::value && !__addr_in_range(*__first))
     {
----------------
Should you make my suggested change right now? Or are you planning to make it in whatever subsequent PR adds `_LIBCPP_CONSTEXPR_AFTER_CXX17` to this `insert` method?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119633