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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


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


================
Comment at: libcxx/include/string:829-832
+#if _LIBCPP_DEBUG_LEVEL == 2
+    if (!__libcpp_is_constant_evaluated())
       __get_db()->__insert_c(this);
+#endif
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115788