__wrap_iter is currently only constexpr if it's not a debug built, but it isn't used in a constexpr context currently. Making it always constexpr and disabling the debugging utilities at constant evaluation is more usful since it has to be always constexpr to be used in a constexpr context.
Details
- Reviewers
ldionne • Quuxplusone Mordante - Group Reviewers
Restricted Project - Commits
- rG6146e4cf89db: [libc++] Make __wrap_iter constexpr
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This seems like probably a good idea.
Can you expand on whether this is strictly needed for the constexpr-string and constexpr-vector stuff, or just a nice-to-have, or what? Basically what doesn't work today, that this enables? (And can we regression-test that thing whatever it is?)
libcxx/include/__iterator/wrap_iter.h | ||
---|---|---|
42–44 | Pre-existing: This > 11 smells like a typo for >= 11; could you take a look at what breaks if you make it >= 11? | |
46–50 | Throughout: Is it time to factor these conditions out into their own helper macro, something like what we did in _LIBCPP_DEBUG_RANDOMIZE_RANGE? #if _LIBCPP_DEBUG_LEVEL == 2 # define _LIBCPP_DEBUG_ITERATORS(...) do { if (!__libcpp_is_constant_evaluated()) { __VA_ARGS__; } } while (0) # define _LIBCPP_DEBUG_ITERATORS_ASSERT(x, m) _LIBCPP_DEBUG_ITERATORS(_LIBCPP_ASSERT(x, m)) #else # define _LIBCPP_DEBUG_ITERATORS(...) (void)0 # define _LIBCPP_DEBUG_ITERATORS_ASSERT(x, m) (void)0 #endif _LIBCPP_DEBUG_ITERATORS(__get_db()->__insert_i(this)); _LIBCPP_DEBUG_ITERATORS_ASSERT(__get_const_db()->__dereferenceable(this), "Attempted to dereference a non-dereferenceable iterator"); This would shorten the code by taking 5 lines down to 2 in a lot of cases. However, I'm amenable to "You Ain't Gonna Need It (yet)" counterarguments. :) | |
95 | Pre-existing: pointer operator should be pointer operator | |
213 | FWIW, this and many following templates can just be _LIBCPP_CONSTEXPR. |
Are there any tests that we can now enable even in debug mode? For example tests that were testing for constexpr-ness that were XFAILed under the debug mode but that will now work?
Other than enabling tests that can be enabled, this LGTM.
libcxx/include/__iterator/wrap_iter.h | ||
---|---|---|
46–50 | I would argue against doing this just yet. I'm going to play around with the debug mode soon and a lot of stuff might change -- I wouldn't do anything in that area just yet. |
I don't know if it is strictly needed, but it is the best way to make the constexpr string work in debug mode.
libcxx/include/__iterator/wrap_iter.h | ||
---|---|---|
72–76 | @philnik: https://github.com/llvm/llvm-project/issues/52902 |
Pre-existing: This > 11 smells like a typo for >= 11; could you take a look at what breaks if you make it >= 11?
(Relatedly, it took me a moment to see why this has to be _LIBCPP_CONSTEXPR_AFTER_CXX11 throughout: all the function bodies now contain if statements, which isn't allowed in C++11 constexpr. That's OK by me.)