Skip to content

[libc++] Always initialize __tree::{,const_}iterator #147167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

philnik777
Copy link
Contributor

@philnik777 philnik777 commented Jul 5, 2025

This was probably added to support https://wg21.link/n3644 but there's no reason not to initialize the pointer in all standard modes. This is technically an extension since n3644 only required value-initialized iterators to be comparable, but supporting this as an extension should be uncontroversial since it avoids potential reads of uninitialized memory in C++03/C++11 without doing any harm.

@philnik777 philnik777 marked this pull request as ready for review July 6, 2025 11:35
@philnik777 philnik777 requested a review from a team as a code owner July 6, 2025 11:35
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 6, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 6, 2025

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

There doesn't seem to be much of a reason to not initialize the iterator types in all standard modes. This avoids potential reads of uninitialized memory in C++03/C++11 without any harm I can see.


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

3 Files Affected:

  • (modified) libcxx/include/__tree (+2-12)
  • (added) libcxx/test/libcxx/containers/associative/map/initialize_iterator.pass.cpp (+26)
  • (added) libcxx/test/libcxx/containers/associative/set/initialize_iterator.pass.cpp (+26)
diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index 9d4ba3ad0639c..5dee314b847f2 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -646,12 +646,7 @@ public:
   using reference         = value_type&;
   using pointer           = __rebind_pointer_t<_NodePtr, value_type>;
 
-  _LIBCPP_HIDE_FROM_ABI __tree_iterator() _NOEXCEPT
-#if _LIBCPP_STD_VER >= 14
-      : __ptr_(nullptr)
-#endif
-  {
-  }
+  _LIBCPP_HIDE_FROM_ABI __tree_iterator() _NOEXCEPT : __ptr_(nullptr) {}
 
   _LIBCPP_HIDE_FROM_ABI reference operator*() const { return __get_np()->__value_; }
   _LIBCPP_HIDE_FROM_ABI pointer operator->() const { return pointer_traits<pointer>::pointer_to(__get_np()->__value_); }
@@ -720,12 +715,7 @@ public:
   using reference         = const value_type&;
   using pointer           = __rebind_pointer_t<_NodePtr, const value_type>;
 
-  _LIBCPP_HIDE_FROM_ABI __tree_const_iterator() _NOEXCEPT
-#if _LIBCPP_STD_VER >= 14
-      : __ptr_(nullptr)
-#endif
-  {
-  }
+  _LIBCPP_HIDE_FROM_ABI __tree_const_iterator() _NOEXCEPT : __ptr_(nullptr) {}
 
 private:
   typedef __tree_iterator<_Tp, __node_pointer, difference_type> __non_const_iterator;
diff --git a/libcxx/test/libcxx/containers/associative/map/initialize_iterator.pass.cpp b/libcxx/test/libcxx/containers/associative/map/initialize_iterator.pass.cpp
new file mode 100644
index 0000000000000..cfa8e72e98e51
--- /dev/null
+++ b/libcxx/test/libcxx/containers/associative/map/initialize_iterator.pass.cpp
@@ -0,0 +1,26 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+
+// Check that map::iterator is initialized when default constructed
+
+#include <cassert>
+#include <map>
+
+template <class Iter>
+void test() {
+  Iter iter;
+  Iter iter2 = Iter();
+  assert(iter == iter2);
+}
+
+int main() {
+  test<std::map<int, int>::iterator>();
+  test<std::map<int, int>::const_iterator>();
+}
diff --git a/libcxx/test/libcxx/containers/associative/set/initialize_iterator.pass.cpp b/libcxx/test/libcxx/containers/associative/set/initialize_iterator.pass.cpp
new file mode 100644
index 0000000000000..c919e5c3680c3
--- /dev/null
+++ b/libcxx/test/libcxx/containers/associative/set/initialize_iterator.pass.cpp
@@ -0,0 +1,26 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+
+// Check that set::iterator is initialized when default constructed
+
+#include <cassert>
+#include <set>
+
+template <class Iter>
+void test() {
+  Iter iter;
+  Iter iter2 = Iter();
+  assert(iter == iter2);
+}
+
+int main() {
+  test<std::set<int>::iterator>();
+  test<std::set<int>::const_iterator>();
+}

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with comments applied.

@@ -0,0 +1,26 @@
//===----------------------------------------------------------------------===//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is effectively testing an extension since https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3644.pdf mentions value-initialized iterators and this extension makes it work with default initialized iterators. Let's move it under test/extensions and adjust the test comment to mention that this is an extension.

@@ -0,0 +1,26 @@
//===----------------------------------------------------------------------===//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing a similar test for multiset and multimap.

assert(iter == iter2);
}

int main() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int main() {
int main(int, char**) {

int main() {
test<std::map<int, int>::iterator>();
test<std::map<int, int>::const_iterator>();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return 0;


template <class Iter>
void test() {
Iter iter;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it, let's test the following combinations:

{
  Iter iter1, iter2;
  assert(iter1 == iter2);
}
{
  Iter iter1;
  Iter iter2{};
  assert(iter1 == iter2);  
}
{
  Iter iter1{};
  Iter iter2;
  assert(iter1 == iter2);  
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants