[libcxx-commits] [PATCH] D115788: [libc++] Disable debug bookkeeping during constant evaluation in <string>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__debug:37
 #elif _LIBCPP_DEBUG_LEVEL == 2
-#   define _LIBCPP_DEBUG_ASSERT(x, m) _LIBCPP_ASSERT(x, m)
+#   define _LIBCPP_DEBUG_ASSERT(x, m) _LIBCPP_ASSERT(__libcpp_is_constant_evaluated() || (x), m)
 #   define _LIBCPP_ASSERT_IMPL(x, m) ((x) ? (void)0 : _VSTD::__libcpp_debug_function(_VSTD::__libcpp_debug_info(__FILE__, __LINE__, #x, m)))
----------------
philnik wrote:
> Quuxplusone wrote:
> > This feels like a change in meaning, from "Assert in debug mode" to "Assert specifically concerned with //debug iterators//." It turns out that this macro has pretty much always been used with the latter meaning, though, so even though a better name might be `_LIBCPP_DEBUG_ITERATOR_ASSERT`, I think this is fine.
> > 
> > However, the one place in libc++ that //doesn't// use the macro with the latter meaning, needs to be updated to use `_LIBCPP_ASSERT` instead.
> > 
> > ```
> > ../libcxx/include/bit:    _LIBCPP_DEBUG_ASSERT(__libcpp_is_constant_evaluated() || __n != numeric_limits<_Tp>::digits, "Bad input to bit_ceil");
> > ```
> > 
> > I think this should just be `_LIBCPP_ASSERT(__n != numeric_limits<_Tp>::digits, "Bad input to bit_ceil");` and if that's not right then investigation is needed as to //why// that's not right.
> > 
> > Then, please update the summary/commit-message to talk about the semantic change here, which I think is more important than the <string> part. Something like, "`_LIBCPP_DEBUG_ASSERT` is now used strictly for debug iterators, and disabled during constant evaluation."  Observe that this is a change for both `<__hash_table>` (since ages ago) //and// `<string>` (since D115765).
> There are not that many usages of `_LIBCPP_DEBUG_ASSERT`. Maybe it would be good to change the meaning  and rename it to `_LIBCPP_DEBUG_ITERATOR_ASSERT` in another patch?
IMO this is fine, but needs to be called out in the commit message. Basically, the important thing this patch is doing is that we're not going to check debug-only assertions inside constant expressions anymore. I think that's acceptable, since those assertions are meant to catch things like UB, and the compiler will do that for us during constexpr evaluation.


================
Comment at: libcxx/include/string:829-832
+#if _LIBCPP_DEBUG_LEVEL == 2
+    if (!__libcpp_is_constant_evaluated())
       __get_db()->__insert_c(this);
+#endif
----------------
philnik wrote:
> Quuxplusone wrote:
> > Somewhere in the past year I'd suggested something like
> > ```
> >     _LIBCPP_IF_DEBUG_ITERATORS(
> >       __get_db()->__insert_c(this);
> >     )
> > ```
> > which could simplify this repeated pattern. Thoughts on reviving that idea?
> I do quite like the idea of `_LIBCPP_IF_DEBUG_ITERATORS`. Should I make this a more general patch to simplify the debug iterators across the repo, or would that be too much?
I am leery of adding a bunch of macros as "shortcuts" for these things. I think the current approach is fine.


================
Comment at: libcxx/include/string:3498
 #if _LIBCPP_DEBUG_LEVEL == 2
+  if (!__libcpp_is_constant_evaluated()) {
     if (!__is_long())
----------------
After your patch, this is a mix of 4 space and 2 space indentation. Please consistently use 4 space in this file, since that's what is done for the other functions in that file.

This applies to all the other functions you touched AFAICT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115788