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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

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.



================
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
     }
----------------
Quuxplusone wrote:
> 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. :)
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114733