-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[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. |
46d0bb9 to
9ea718f
Compare
|
At this point, I have fixed all the trivially fixable issues as far as I can tell, but naturally trying to fix the lifetime issue I saw when trying to fix it in: (need explicit constructors for And that has resulted in a im-perfect forwarding (I think) I did find this commit that mentions that the code is UB (slightly less than previously), so maybe |
This is clearly UB in the core language. Per current rules we can't assign the IIUC we need to perform different operations (e.g. reconstructing the node) in constant evaluation. |
|
Update: Need to wait for #134819 to be merged before this PR can proceed. That PR should get rid of some more UB, and later I should be able to:
From Discord by philnik777 |
…P3372-constexpr-map
7906780 to
0cce96e
Compare
libcxx/include/__tree
Outdated
| // Clang doesn't optimize on this currently though. | ||
| const_cast<__key_type&>(__lhs.first) = const_cast<__copy_cvref_t<_From, __key_type>&&>(__rhs.first); | ||
| __lhs.second = std::forward<_From>(__rhs).second; | ||
| if (std::is_constant_evaluated()) { |
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.
<__type_traits/is_constant_evaluated.h> should be included for this. If you decide not to guard this with _LIBCXX_STD_VER >= 26, __libcpp_is_constant_evaluated() should be used instead.
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 since the method itself is marked as _LIBCPP_CONSTEXPR_SINCE_CXX26 the extra guard may not be needed and it should be fine to use the std::is_constant_evaluated() variant as this would be the first time this method is valid to be used in constant evaluation.
https://godbolt.org/z/b91EK95hj
edit:
never mind, just realized, it won't compile below cpp20 probably if i use the std:: version
libcxx/include/__tree
Outdated
| // tmp.second = std::forward<_From>(__rhs).second; | ||
| // __lhs = pair<const int, double> | ||
| // TODO: use a better name | ||
| using foo_type = __get_node_value_type_t<value_type>; |
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.
| using foo_type = __get_node_value_type_t<value_type>; | |
| using __node_value_type = __get_node_value_type_t<value_type>; |
libcxx/include/__tree
Outdated
| _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX26 void | ||
| __insert_unique_from_orphaned_node(const_iterator __p, __get_node_value_type_t<_Tp>&& __value) { | ||
| using __key_type = typename _NodeTypes::key_type; | ||
| // fails here for move_alloc.pass.cpp |
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.
There seem to be obstacles that can't be worked around. I guess we should skip some test cases for constant evaluation at this moment, and submit an LWG issue (even though there's already CWG2514).
8658eee to
66b4d6f
Compare
|
The latest failure: seems to be related to this: #152724 not sure why that is failing however |
…P3372-constexpr-map
|
@frederick-vs-ja @philnik777 @H-G-Hristov @Zingam Can you take a look at this pull request when you have a chance? Thank you! |
…P3372-constexpr-map
…P3372-constexpr-map
1118540 to
4af614d
Compare
…ay-issue-128660-P3372-constexpr-map
Fixes #128660
Depends on:
libcxx/include/__memory/pointer_traits.h#157304std::__tree_nodeconstruction #153908Summary:
Apply
_LIBCPP_CONSTEXPR_SINCE_CXX26tomap,__tree,node-handle,__libcpp_erase_if,__lazy_synth_three_way_comparator,__lazy_compare_result,libcxx/include/__utility/try_key_extraction.hSkip test
erase_if.pass.cppfor GCC during constant evaluation. AFAICT this appears to be a g++ bug, as the test passes with clangDisable test for
node-handle::key()during constant evaluation (CWG2514). It is annotated with a// FIXMEmap.modifiers/try.emplace.pass.cpp: Start using the previously unusedmv3. (Should this be a separate patch?)Has a TODO for
multimapand ohtersa.
libcxx/test/std/containers/associative/map/map.ops/contains.pass.cppb.
libcxx/test/std/containers/associative/map/map.ops/contains_transparent.pass.cppa.
libcxx/test/std/containers/container.node/node_handle.pass.cppFix typo in-> [NFC][libc++] Fix typo inlibcxx/include/__memory/pointer_traits.hlibcxx/include/__memory/pointer_traits.h#157304pair<const MoveOnly, ...>a.
move_assign.pass.cppb.
move_alloc.pass.cppc. Fails to compile if
static_assert(test());is called in the test filed. Has a
// FIXMEwith commented codeChangeThis became -> [libc++] Remove UB from__tree_nodeto construct it's__node_value_typeduring construction to avoid the:note: member call on object outside its lifetime is not allowed in a constant expressionissue.std::__tree_nodeconstruction #153908