This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make __wrap_iter constexpr
ClosedPublic

Authored by philnik on Nov 29 2021, 12:10 PM.

Details

Reviewers
ldionne
Quuxplusone
Mordante
Group Reviewers
Restricted Project
Commits
rG6146e4cf89db: [libc++] Make __wrap_iter constexpr
Summary

__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.

Diff Detail

Event Timeline

philnik requested review of this revision.Nov 29 2021, 12:10 PM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2021, 12:10 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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?
(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.)

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.
Argument pro: Shorter, and can be updated to just constexpr as soon as we drop C++03 support
Argument con: Consistency.

ldionne accepted this revision.Nov 29 2021, 1:48 PM

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.

This revision is now accepted and ready to land.Nov 29 2021, 1:48 PM
philnik updated this revision to Diff 390522.Nov 29 2021, 4:41 PM
philnik marked 4 inline comments as done.

Minor changes

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?)

I don't know if it is strictly needed, but it is the best way to make the constexpr string work in debug mode.

philnik updated this revision to Diff 390650.Nov 30 2021, 3:03 AM

Initializing pointer in all C++ versions now

This revision was automatically updated to reflect the committed changes.
libcxx/include/__iterator/wrap_iter.h
72–76

@philnik: https://github.com/llvm/llvm-project/issues/52902
Could you take a look when you get a chance? (And scan for any other similar bugs that might be hiding elsewhere in here.)