[libcxx-commits] [PATCH] D115788: [libc++] Disable debug bookkeeping during constant evaluation in <string>
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Dec 15 06:16:00 PST 2021
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
More information about the libcxx-commits
mailing list