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

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

Quuxplusone 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)))
----------------
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).


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


================
Comment at: libcxx/include/string:2990
 {
+
   _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(&__pos) == this,
----------------
Stray blank line here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115788