[libc++] Ensure strong exception guarantee for forward_list::resize

[winner245]

The current implementation of forward_list::resize does not meet the strong exception safety guarantee required by [forward.list.modifiers]: If an exception is thrown by any of these member functions there is no effect on the container. This patch refactors resize() to provide strong exception safety and introduces additional tests to validate the strong exception guarantees for other forward_list modifiers.

Fixes #118366.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

The current implementation of forward_list::resize does not meet the strong exception safety guarantee required by [forward.list.modifiers]: If an exception is thrown by any of these member functions there is no effect on the container. This patch refactors resize() to provide strong exception safety and introduces additional tests to validate the strong exception guarantees for other forward_list modifiers.

Fixes #118366.

---

Full diff: https://github.com/llvm/llvm-project/pull/131025.diff

4 Files Affected:

• (modified) libcxx/docs/Status/Cxx2cIssues.csv (+1-1)
• (modified) libcxx/include/forward_list (+33-12)
• (modified) libcxx/test/std/containers/exception_safety_helpers.h (+51)
• (modified) libcxx/test/std/containers/sequences/forwardlist/exception_safety.pass.cpp (+54-19)

diff --git a/libcxx/docs/Status/Cxx2cIssues.csv b/libcxx/docs/Status/Cxx2cIssues.csv
index 9bf31c417f3c9..fdf381862d87b 100644
--- a/libcxx/docs/Status/Cxx2cIssues.csv
+++ b/libcxx/docs/Status/Cxx2cIssues.csv
@@ -107,7 +107,7 @@
 "`LWG4153 <https://wg21.link/LWG4153>`__","Fix extra ""-1"" for ``philox_engine::max()``","2024-11 (Wrocław)","","",""
 "`LWG4154 <https://wg21.link/LWG4154>`__","The Mandates for ``std::packaged_task``'s constructor from a callable entity should consider decaying","2024-11 (Wrocław)","","",""
 "`LWG4157 <https://wg21.link/LWG4157>`__","The resolution of LWG3465 was damaged by P2167R3","2024-11 (Wrocław)","","20",""
-"`LWG4164 <https://wg21.link/LWG4164>`__","Missing guarantees for ``forward_list`` modifiers","2024-11 (Wrocław)","","",""
+"`LWG4164 <https://wg21.link/LWG4164>`__","Missing guarantees for ``forward_list`` modifiers","2024-11 (Wrocław)","|Complete|","21",""
 "`LWG4169 <https://wg21.link/LWG4169>`__","``std::atomic<T>``'s default constructor should be constrained","2024-11 (Wrocław)","","",""
 "`LWG4170 <https://wg21.link/LWG4170>`__","``contiguous_iterator`` should require ``to_address(I{})``","2024-11 (Wrocław)","","",""
 "","","","","",""
diff --git a/libcxx/include/forward_list b/libcxx/include/forward_list
index 7582de20995b9..6cad1f96f7019 100644
--- a/libcxx/include/forward_list
+++ b/libcxx/include/forward_list
@@ -819,6 +819,7 @@ public:
 
   template <class _InputIterator, class _Sentinel>
   _LIBCPP_HIDE_FROM_ABI iterator __insert_after_with_sentinel(const_iterator __p, _InputIterator __f, _Sentinel __l);
+  _LIBCPP_HIDE_FROM_ABI iterator __default_insert_after(const_iterator __p, size_type __n);
 
   _LIBCPP_HIDE_FROM_ABI iterator erase_after(const_iterator __p);
   _LIBCPP_HIDE_FROM_ABI iterator erase_after(const_iterator __f, const_iterator __l);
@@ -1128,6 +1129,36 @@ forward_list<_Tp, _Alloc>::insert_after(const_iterator __p, const value_type& __
   return iterator(__r->__next_);
 }
 
+template <class _Tp, class _Alloc>
+typename forward_list<_Tp, _Alloc>::iterator
+forward_list<_Tp, _Alloc>::__default_insert_after(const_iterator __p, size_type __n) {
+  __begin_node_pointer __r = __p.__get_begin();
+  if (__n > 0) {
+    __node_pointer __first = this->__create_node(/* next = */ nullptr);
+    __node_pointer __last  = __first;
+#  if _LIBCPP_HAS_EXCEPTIONS
+    try {
+#  endif // _LIBCPP_HAS_EXCEPTIONS
+      for (--__n; __n != 0; --__n, __last = __last->__next_) {
+        __last->__next_ = this->__create_node(/* next = */ nullptr);
+      }
+#  if _LIBCPP_HAS_EXCEPTIONS
+    } catch (...) {
+      while (__first != nullptr) {
+        __node_pointer __next = __first->__next_;
+        this->__delete_node(__first);
+        __first = __next;
+      }
+      throw;
+    }
+#  endif // _LIBCPP_HAS_EXCEPTIONS
+    __last->__next_ = __r->__next_;
+    __r->__next_    = __first;
+    __r             = static_cast<__begin_node_pointer>(__last);
+  }
+  return iterator(__r);
+}
+
 template <class _Tp, class _Alloc>
 typename forward_list<_Tp, _Alloc>::iterator
 forward_list<_Tp, _Alloc>::insert_after(const_iterator __p, size_type __n, const value_type& __v) {
@@ -1240,12 +1271,7 @@ void forward_list<_Tp, _Alloc>::resize(size_type __n) {
   if (__i != __e)
     erase_after(__p, __e);
   else {
-    __n -= __sz;
-    if (__n > 0) {
-      for (__begin_node_pointer __ptr = __p.__get_begin(); __n > 0; --__n, __ptr = __ptr->__next_as_begin()) {
-        __ptr->__next_ = this->__create_node(/* next = */ nullptr);
-      }
-    }
+    __default_insert_after(__p, __n - __sz);
   }
 }
 
@@ -1260,12 +1286,7 @@ void forward_list<_Tp, _Alloc>::resize(size_type __n, const value_type& __v) {
   if (__i != __e)
     erase_after(__p, __e);
   else {
-    __n -= __sz;
-    if (__n > 0) {
-      for (__begin_node_pointer __ptr = __p.__get_begin(); __n > 0; --__n, __ptr = __ptr->__next_as_begin()) {
-        __ptr->__next_ = this->__create_node(/* next = */ nullptr, __v);
-      }
-    }
+    insert_after(__p, __n - __sz, __v);
   }
 }
 
diff --git a/libcxx/test/std/containers/exception_safety_helpers.h b/libcxx/test/std/containers/exception_safety_helpers.h
index da247571d303f..2d26735d9bb4e 100644
--- a/libcxx/test/std/containers/exception_safety_helpers.h
+++ b/libcxx/test/std/containers/exception_safety_helpers.h
@@ -11,6 +11,7 @@
 
 #include <cassert>
 #include <cstddef>
+#include <forward_list>
 #include <functional>
 #include <utility>
 #include "test_macros.h"
@@ -53,6 +54,33 @@ int ThrowingCopy<N>::created_by_copying = 0;
 template <int N>
 int ThrowingCopy<N>::destroyed = 0;
 
+template <int N>
+struct ThrowingDefault {
+  static bool throwing_enabled;
+  static int default_constructed;
+  static int destroyed;
+  int x = 0;
+
+  ThrowingDefault() {
+    ++default_constructed;
+    if (throwing_enabled && default_constructed == N) {
+      throw -1;
+    }
+  }
+
+  ThrowingDefault(int value) : x(value) {}
+  ThrowingDefault(const ThrowingDefault& other) = default;
+  friend bool operator==(const ThrowingDefault& lhs, const ThrowingDefault& rhs) { return lhs.x == rhs.x; }
+  friend bool operator<(const ThrowingDefault& lhs, const ThrowingDefault& rhs) { return lhs.x < rhs.x; }
+
+  static void reset() { default_constructed = destroyed = 0; }
+};
+
+template <int N>
+bool ThrowingDefault<N>::throwing_enabled = true;
+template <int N>
+int ThrowingDefault<N>::default_constructed = 0;
+
 template <int N>
 struct std::hash<ThrowingCopy<N>> {
   std::size_t operator()(const ThrowingCopy<N>& value) const { return value.x; }
@@ -95,6 +123,29 @@ void test_exception_safety_throwing_copy_container(Func&& func) {
   }
 }
 
+template <int ThrowOn, int Size, class Func>
+void test_strong_exception_safety_throwing_copy(Func&& func) {
+  using T             = ThrowingCopy<ThrowOn>;
+  T::throwing_enabled = false;
+
+  std::forward_list<T> c0(Size);
+  for (int i = 0; i < Size; ++i)
+    c0.emplace_front(i);
+  std::forward_list<T> c = c0;
+  T in[Size];
+  T::reset();
+  T::throwing_enabled = true;
+  try {
+    func(c, in, in + Size);
+    assert(false); // The function call above should throw.
+
+  } catch (int) {
+    assert(T::created_by_copying == ThrowOn);
+    assert(T::destroyed == ThrowOn - 1); // No destructor call for the partially-constructed element.
+    assert(c == c0);                     // Strong exception guarantee
+  }
+}
+
 #endif // !defined(TEST_HAS_NO_EXCEPTIONS)
 
 #endif // SUPPORT_EXCEPTION_SAFETY_HELPERS_H
diff --git a/libcxx/test/std/containers/sequences/forwardlist/exception_safety.pass.cpp b/libcxx/test/std/containers/sequences/forwardlist/exception_safety.pass.cpp
index faf42202e951a..a6579e144e6d3 100644
--- a/libcxx/test/std/containers/sequences/forwardlist/exception_safety.pass.cpp
+++ b/libcxx/test/std/containers/sequences/forwardlist/exception_safety.pass.cpp
@@ -35,18 +35,23 @@
 //     void assign_range(R&& rg); // C++23
 //
 // void push_front(const value_type& v);
-//  template<container-compatible-range<T> R>
+// template <class... Args> reference emplace_front(Args&&... args);  // reference in C++17
+// template<container-compatible-range<T> R>
 //    void prepend_range(R&& rg); // C++23
 //
 // iterator insert_after(const_iterator p, const value_type& v);
+// iterator insert_after(const_iterator p, value_type&& v);
 // iterator insert_after(const_iterator p, size_type n, const value_type& v);
+// iterator insert_after(const_iterator p, initializer_list<value_type> il);
 // template <class InputIterator>
 //     iterator insert_after(const_iterator p,
 //                           InputIterator first, InputIterator last);
-//  template<container-compatible-range<T> R>
+// template<container-compatible-range<T> R>
 //     iterator insert_range_after(const_iterator position, R&& rg); // C++23
-//
+// template <class... Args>
+//     iterator emplace_after(const_iterator p, Args&&... args);
 // void resize(size_type n, const value_type& v);
+// void resize(size_type n);
 
 #include <forward_list>
 
@@ -65,16 +70,33 @@ int main(int, char**) {
     using T               = ThrowingCopy<ThrowOn>;
 
     // void push_front(const value_type& v);
-    test_exception_safety_throwing_copy<ThrowOn, Size>([](T* from, T*) {
-      std::forward_list<T> c;
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T*) {
+      c.push_front(*from);
+    });
+
+    // template <class... Args> reference emplace_front(Args&&... args);
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T*) {
       c.push_front(*from);
     });
 
     // iterator insert_after(const_iterator p, const value_type& v);
-    test_exception_safety_throwing_copy</*ThrowOn=*/1, Size>([](T* from, T*) {
-      std::forward_list<T> c;
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T*) {
       c.insert_after(c.before_begin(), *from);
     });
+
+    // iterator insert_after(const_iterator p, value_type&& v);
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T*) {
+      c.insert_after(c.before_begin(), std::move(*from));
+    });
+
+    // template <class... Args>
+    //     iterator emplace_after(const_iterator p, Args&&... args);
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T*) {
+      c.emplace_after(c.before_begin(), *from);
+    });
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T*) {
+      c.emplace_after(c.before_begin(), std::move(*from));
+    });
   }
 
   {
@@ -169,40 +191,53 @@ int main(int, char**) {
 #if TEST_STD_VER >= 23
     // template<container-compatible-range<T> R>
     //   void prepend_range(R&& rg); // C++23
-    test_exception_safety_throwing_copy<ThrowOn, Size>([](T* from, T* to) {
-      std::forward_list<T> c;
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T* to) {
       c.prepend_range(std::ranges::subrange(from, to));
     });
 #endif
 
     // iterator insert_after(const_iterator p, size_type n, const value_type& v);
-    test_exception_safety_throwing_copy<ThrowOn, Size>([](T* from, T*) {
-      std::forward_list<T> c;
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T*) {
       c.insert_after(c.before_begin(), Size, *from);
     });
 
     // template <class InputIterator>
     //     iterator insert_after(const_iterator p,
     //                           InputIterator first, InputIterator last);
-    test_exception_safety_throwing_copy<ThrowOn, Size>([](T* from, T* to) {
-      std::forward_list<T> c;
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T* to) {
       c.insert_after(c.before_begin(), from, to);
     });
 
+    // iterator insert_after(const_iterator p, initializer_list<value_type> il);
+    std::initializer_list<T> il{1, 2, 3, 4, 5};
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([&](std::forward_list<T>& c, T*, T*) {
+      c.insert_after(c.before_begin(), il);
+    });
+
 #if TEST_STD_VER >= 23
     // template<container-compatible-range<T> R>
     //     iterator insert_range_after(const_iterator position, R&& rg); // C++23
-    test_exception_safety_throwing_copy<ThrowOn, Size>([](T* from, T* to) {
-      std::forward_list<T> c;
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T* to) {
       c.insert_range_after(c.before_begin(), std::ranges::subrange(from, to));
     });
 #endif
 
     // void resize(size_type n, const value_type& v);
-    test_exception_safety_throwing_copy<ThrowOn, Size>([](T* from, T*) {
-      std::forward_list<T> c;
-      c.resize(Size, *from);
-    });
+    test_strong_exception_safety_throwing_copy<ThrowOn, Size>([](std::forward_list<T>& c, T* from, T*) {
+      c.resize(Size * 3, *from);
+    });
+
+    { // void resize(size_type n);
+      using X = ThrowingDefault<ThrowOn>;
+      std::forward_list<X> c0{X{1}, X{2}, X{3}};
+      std::forward_list<X> c = c0;
+      try {
+        c.resize(3 * ThrowOn);
+        assert(false);
+      } catch (int) {
+        assert(c == c0);
+      }
+    }
   }
 
   return 0;

[frederick-vs-ja]

As __default_insert_after is only used once, it seems better to me to inline it into resize.

[winner245]

Thank you for the suggestion! I‘ve inlined __default_insert_after into resize.

[ldionne]

LGTM, thanks. I do have a comment I'd like you to consider since it might simplify the code and reduce duplication, but if that doesn't pan out, this patch LGTM as is.