Skip to content

Conversation

@AlexGuteniev
Copy link
Contributor

Per [sequence.reqmts] there are these member functions.

I did not audit if any other member functions are missing. Adding these is enough for MSVC STL

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner May 16, 2025 17:53
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 16, 2025
@llvmbot
Copy link
Member

llvmbot commented May 16, 2025

@llvm/pr-subscribers-libcxx

Author: Alex Guteniev (AlexGuteniev)

Changes

Per [sequence.reqmts] there are these member functions.

I did not audit if any other member functions are missing. Adding these is enough for MSVC STL


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

1 Files Affected:

  • (modified) libcxx/test/support/MinSequenceContainer.h (+9)
diff --git a/libcxx/test/support/MinSequenceContainer.h b/libcxx/test/support/MinSequenceContainer.h
index 6e61aff06344b..b17ced6dfc2ff 100644
--- a/libcxx/test/support/MinSequenceContainer.h
+++ b/libcxx/test/support/MinSequenceContainer.h
@@ -28,6 +28,13 @@ struct MinSequenceContainer {
   template <class It>
   explicit MinSequenceContainer(It first, It last) : data_(first, last) {}
   MinSequenceContainer(std::initializer_list<T> il) : data_(il) {}
+
+  template <class It>
+  void assign(It first, It last) {
+    data_.assign(first, last);
+  }
+  void assign(std::initializer_list<T> il) { data_.assign(il); }
+  void assign(size_type n, value_type t) { data_.assign(n, t); }
   iterator begin() { return iterator(data_.data()); }
   const_iterator begin() const { return const_iterator(data_.data()); }
   const_iterator cbegin() const { return const_iterator(data_.data()); }
@@ -47,6 +54,8 @@ struct MinSequenceContainer {
     return from_vector_iterator(data_.insert(to_vector_iterator(p), std::move(value)));
   }
 
+  iterator insert_range(const_iterator p, auto rg) { return data_.insert_range((to_vector_iterator(p), rg); }
+
   iterator erase(const_iterator first, const_iterator last) {
     return from_vector_iterator(data_.erase(to_vector_iterator(first), to_vector_iterator(last)));
   }

@github-actions
Copy link

github-actions bot commented May 16, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@AlexGuteniev AlexGuteniev requested a review from philnik777 May 17, 2025 10:25
@huixie90
Copy link
Member

thanks for the fix. actually i made a commit recently which relies on this helper type's absence of insert_range
4b104c6#diff-0c32ebccc1456e2143518d2ce055db04535240cab1881f8f3412362f8ae22521

this means i need to refactor the test not to use MinSenquenceContainer to make CI green.

@AlexGuteniev
Copy link
Contributor Author

actually i made a commit recently which relies on this helper type's absence of insert_range

Maybe we can solve this with prerocessor then

@AlexGuteniev
Copy link
Contributor Author

Maybe we can solve this with prerocessor then

Or maybe we should consider supporting this too.

@StephanTLavavej , please confirm if supporting pre-C++23 containers in <flat_set> is non-goal for MSVC STL

@huixie90
Copy link
Member

@AlexGuteniev I fixed my tests not relying on MinSequenceContainer
#140372

@StephanTLavavej
Copy link
Member

please confirm if supporting pre-C++23 containers in <flat_set> is non-goal for MSVC STL

I'd say yes, that's a non-goal. It seems to me that if someone wants to use a new container adaptor, they can be expected to provide a sufficiently modern container.

@AlexGuteniev
Copy link
Contributor Author

@AlexGuteniev I fixed my tests not relying on MinSequenceContainer

Thanks, but I don't think it is a path forward. The goal of my PR is to make the tests, including yours, pass with MSVC STL, so with your fix my PR does not make sense.

I think we should instead use preprocessor and make insert_range unavailable when _LIBCPP_VERSION is defined. This matches the practice of LIBCPP_ASSERT macro.

@huixie90
Copy link
Member

huixie90 commented May 17, 2025

@AlexGuteniev I fixed my tests not relying on MinSequenceContainer

Thanks, but I don't think it is a path forward. The goal of my PR is to make the tests, including yours, pass with MSVC STL, so with your fix my PR does not make sense.

I think we should instead use preprocessor and make insert_range unavailable when _LIBCPP_VERSION is defined. This matches the practice of LIBCPP_ASSERT macro.

No I don't think you should guard against libc++. Your original fix looks good to me. the helper type in the test/std folder is meant to be used for all implementations. not just libc++.

Anything that is libc++ specific is our extension and should be put into test/libcxx folder. and this is what I was doing in #140372

@AlexGuteniev
Copy link
Contributor Author

No I don't think you should guard against libc++. Your original fix looks good to me.

Ok, will revert that

Copy link
Member

@huixie90 huixie90 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 green CI

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

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

I did not audit if any other member functions are missing. Adding these is enough for MSVC STL

The following member functions are still missing:

  • from_range_t constructor (since C++23),
  • constructor from (n, t),
  • operator= from initializer_list,
  • insert taking (p, n, t),
  • insert taking (p, il),
  • assign_range (since C++23).

No change requested. I think they can be added in a following-up PR.

@huixie90 huixie90 self-assigned this May 18, 2025
ldionne pushed a commit that referenced this pull request May 30, 2025
The affected tests are relying on the fact that `MinSequenceContainer`
does not have `insert_range`. This prevents landing of #140287.

This PR creates a new helper class to allow the change in MinSequenceContainer.
@huixie90
Copy link
Member

huixie90 commented Jun 7, 2025

btw, the CI looks OK to me. the msan failure has nothing to do with your patch

MemorySanitizer: CHECK failed: msan_linux.cpp:193 "((personality(old_personality | ADDR_NO_RANDOMIZE))) != ((-1))" (0xffffffffffffffff, 0xffffffffffffffff) (tid=5352)

This failure seems happen all the time now.

@frederick-vs-ja frederick-vs-ja merged commit 8631cdd into llvm:main Jun 9, 2025
71 of 81 checks passed
@AlexGuteniev AlexGuteniev deleted the patch-1 branch June 9, 2025 10:36
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
… MSVC STL (llvm#140287)

Per [sequence.reqmts] there are these member functions.

I did not audit if any other member functions are missing. Adding these
is enough for MSVC STL
frederick-vs-ja pushed a commit that referenced this pull request Sep 13, 2025
…pass on MSVC STL (#158344)

Continues #140287

`from_range_t` constructor is needed by MSVC STL to pass
`std/containers/container.adaptors/flat.set/flat.set.cons/range.pass.cpp`.

The rest are added to complete the container according to
#140287 (review).
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Sep 16, 2025
… more test pass on MSVC STL (#158344)

Continues #140287

`from_range_t` constructor is needed by MSVC STL to pass
`std/containers/container.adaptors/flat.set/flat.set.cons/range.pass.cpp`.

The rest are added to complete the container according to
llvm/llvm-project#140287 (review).
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.

6 participants