[libcxx-commits] [PATCH] D114733: [libc++] Make __wrap_iter constexpr

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

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



================
Comment at: libcxx/include/__iterator/wrap_iter.h:43-46
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11 __wrap_iter() _NOEXCEPT
 #if _LIBCPP_STD_VER > 11
                 : __i{}
 #endif
----------------
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.)


================
Comment at: libcxx/include/__iterator/wrap_iter.h:48-52
 #if _LIBCPP_DEBUG_LEVEL == 2
+      if (!__libcpp_is_constant_evaluated())
         __get_db()->__insert_i(this);
 #endif
     }
----------------
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. :)


================
Comment at: libcxx/include/__iterator/wrap_iter.h:97
     }
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG pointer  operator->() const _NOEXCEPT
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11 pointer  operator->() const _NOEXCEPT
     {
----------------
Pre-existing: `pointer  operator` should be `pointer operator`


================
Comment at: libcxx/include/__iterator/wrap_iter.h:215
 template <class _Iter1>
-_LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_IF_NODEBUG
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
 bool operator!=(const __wrap_iter<_Iter1>& __x, const __wrap_iter<_Iter1>& __y) _NOEXCEPT
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114733/new/

https://reviews.llvm.org/D114733