[libcxx-commits] [PATCH] D98573: [libc++] Remove the special logic for "noexcept iterators" in basic_string

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I remember you had a guard that would put back `'\0'` at the end of the string if the operation failed in a previous version of this patch. To be sure I understand, that was only relevant when you tried using the non-allocating fast path with iterators that don't have `noexcept` operations, which you don't do anymore (now those will allocate "naively"). Correct?



================
Comment at: libcxx/include/string:640
+struct __string_is_trivial_iterator<_Tp*>
+    : public is_arithmetic<_Tp> {};
 
----------------
I don't understand why only pointers to arithmetic types are considered trivial here?


================
Comment at: libcxx/include/string:1701
+    template<class _Tp>
+    bool __addr_in_range(_Tp&& __t) const {
+        const volatile void *__p = _VSTD::addressof(__t);
----------------
`_LIBCPP_INLINE_VISIBILTY`


================
Comment at: libcxx/include/string:2161-2176
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    try
+    {
+#endif  // _LIBCPP_NO_EXCEPTIONS
     for (; __first != __last; ++__first, (void) ++__p)
         traits_type::assign(*__p, *__first);
     traits_type::assign(*__p, value_type());
----------------
Quuxplusone wrote:
> @ldionne @EricWF I'd appreciate some extra eyeballs on this part. This memory leak was detected by ASAN: https://buildkite.com/llvm-project/libcxx-ci/builds/2613#fc0d2573-8a43-4b02-b142-009a84ef9b06  I'm not real familiar with the role of `basic_string::__init`; this fix //seems// straightforward, but I wonder if there's a subtle reason someone left it off originally. Compare to lines 2118–2132 above.
My knee jerk reaction would be that it was left off because `traits_type::assign` was thought of as being non-throwing, and nobody thought that the iterator types themselves could throw in their operations. IOW, I would be tempted to say it's an oversight.


================
Comment at: libcxx/include/string:2493
+    if (__string_is_trivial_iterator<_ForwardIterator>::value &&
+        (__cap >= __n || !__addr_in_range(*__first)))
     {
----------------
You can't dereference `__first` here if `__first == __last`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98573