-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[libc++] Make <map>
std::map
constexpr as part of P3372R3
#134330
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
base: main
Are you sure you want to change the base?
[libc++] Make <map>
std::map
constexpr as part of P3372R3
#134330
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
181a08d
to
6560dd5
Compare
…ect of unique_ptr
…use the pointed object type 'std::__tree_node<std::__value_type<int, double>, min_pointer<void>>' is not similar to the target type 'std::__tree_node_base<min_pointer<void>>'
…use the pointed object type 'std::__tree_node<std::__value_type<int, double>, min_pointer<void>>' is not similar to the target type 'std::__tree_node_base<min_pointer<void>>
46d0bb9
to
9ea718f
Compare
libcxx/include/__tree
Outdated
__tree<_Tp, _Compare, _Allocator>::__construct_node(_Args&&... __args) { | ||
__node_allocator& __na = __node_alloc(); | ||
__node_holder __h(__node_traits::allocate(__na, 1), _Dp(__na)); | ||
|
||
#if _LIBCPP_STD_VER >= 26 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use something like
std::__construct_at(std::addressof(__h), __na, std::forward<_Args>(__args)...)
in all modes.
And use __node_traits::construct
in the constructor of __node
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
It works if I don't give the __na
argument, as there is no allocator arg here:
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Tp* __construct_at(_Tp* __location, _Args&&... __args) { |
Q:
Should this line:
llvm-project/libcxx/include/__tree
Line 1768 in 5165a6c
__node_holder __h(__node_traits::allocate(__na, 1), _Dp(__na)); |
actually be
__node_holder __h(__node_traits::allocate(__na, sizeof(typename __node_holder::element_type)), _Dp(__na));
?
If not, how does it work?
Update:
Allocates n * sizeof(T) bytes of uninitialized storage by calling
- https://en.cppreference.com/w/cpp/memory/allocator/allocate.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And use __node_traits::construct in the constructor of __node.
not sure I understand where this is supposed to be as:
is the definition of __node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And use __node_traits::construct in the constructor of __node.
not sure I understand where this is supposed to be as:
is the definition of
__node
Hmm... __node
is actually __tree_node
, so we should modify that class. It seems necessary to wrap the __value_
member into a union
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... __node is actually __tree_node, so we should modify that class. It seems necessary to wrap the _value member into a union.
I think I might be missing something,
Modify the class in what manner? I have added a constructor that was previously missing to __tree_node
so I can construct it during constant evaluation.
It seems necessary to wrap the _value member into a union.
What would be the reason to do this?
Something like:
class __tree_node {
union {
... __value_;
??? // what would be this part?
}
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the reason to do this?
We need to use allocator's construct
function (if any) to construct the element.
// what would be this part?
I think nothing else should be added to the union. We just need to call __node_traits::construct(__a, std::addressof(__value_), __na, std::forward<_Args>(__args)...)
in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand
Let me try this tomorrow.
In all modes
Should this be submitted as a separate patch then? If we're also changing the existing runtime code path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all modes
Should this be submitted as a separate patch then? If we're also changing the existing runtime code path
I think so. But I overlooked C++03 mode before. Perhaps need to do different things (or keep old strategy?) in C++03 mode as some uses of union
might be ill-formed.
<map>
std::map
constexpr as part of P3372R3
…d.`Revert "PR review comment" This reverts commit 4fbb472. Fails with: ``` # .---command stderr------------ # | t.tmp.exe: /home/runner/_work/llvm-project/llvm-project/libcxx/test/support/container_test_types.h:399: CopyInsertable<1>::CopyInsertable(const CopyInsertable<Dummy> &) [Dummy = 1]: Assertion `getConstructController()->isInAllocatorConstruct()' failed. # `----------------------------- # error: command failed with exit status: 250 ```
990228b
to
efd9f96
Compare
Not sure what this failure is in gcc-15 tests:
AIX 32 fails with the following and basically the whole test suite(not sure if it has the up to date compiler) AIX is using Clang 19, so about a year old and that may be the cause of this failure:
|
Fixes #128660
Summary:
Apply
_LIBCPP_CONSTEXPR_SINCE_CXX26
tomap
,__tree
andnode-handle
map.modifiers/try.emplace.pass.cpp
: Start using the previously unusedmv3
. (Should this be a separate patch?)Has a TODO for
multimap
a.
libcxx/test/std/containers/associative/map/map.ops/contains.pass.cpp
b.
libcxx/test/std/containers/associative/map/map.ops/contains_transparent.pass.cpp
TODO: for
multimap
and others:a.
libcxx/test/std/containers/container.node/node_handle.pass.cpp
Fix typo in
libcxx/include/__memory/pointer_traits.h
pair<const MoveOnly, ...>
a.
move_assign.pass.cpp
b.
move_alloc.pass.cpp
c. Fails to compile if
static_assert(test());
is called in the test filed. Has a
// FIXME
with commented codeBased on this comment, I tried replacing:
a.
__node_traits::construct(__na, std::addressof(__h->__value_), std::forward<_Args>(__args)...);
b. with
c.
std::__construct_at(std::addressof(__h->__value_), std::forward<_Args>(__args)...);
d. Fails with: