Skip to content

libc++ test: update MinSequenceContainer.h to make some tests pass on MSVC STL #140287

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

Merged
merged 15 commits into from
Jun 9, 2025

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)));
   }

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
rorth pushed a commit to rorth/llvm-project that referenced this pull request Jun 11, 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
DhruvSrivastavaX pushed a commit to DhruvSrivastavaX/lldb-for-aix that referenced this pull request Jun 12, 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
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
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