[libcxx-commits] [PATCH] D153672: [libc++] Remove the legacy debug mode.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

This LGTM once CI is green and comments are applied. Please ping me one last time and I'll only look at the diff-since-last-change real quick. Thanks for the patch, we've been wanting to do this for a long time!



================
Comment at: libcxx/include/__algorithm/comp_ref_type.h:53
     __do_compare_assert(int, _LHS & __l, _RHS & __r) {
-        _LIBCPP_DEBUG_ASSERT(!__comp_(__l, __r),
+        _LIBCPP_ASSERT(!__comp_(__l, __r),
             "Comparator does not induce a strict weak ordering");
----------------
Should this be `UNCATEGORIZED_ASSERT` instead? Assuming you land the patch that does the renaming first, I think it should.


================
Comment at: libcxx/include/__tree:379-382
+    // TODO(varconst): make this work with the new debug mode (or remove).
+#ifdef _LIBCPP_ENABLE_DEBUG_MODE
+    _LIBCPP_ASSERT(std::__tree_invariant(__root), "The tree invariants should hold");
+#endif
----------------
I think this should just be a `_LIBCPP_UNCATEGORIZED_ASSERT` for now. It would work today.

Once we categorize this, this could become something like `_LIBCPP_INTERNAL_ASSERTION` or something like that since it's effectively an internal consistency check to ensure that our implementation isn't broken. We have other instances of that, and we had thought about adding another assertion macro to handle those back then.


================
Comment at: libcxx/include/vector:1736
 {
-    _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(std::addressof(__position)) == this,
-                         "vector::erase(iterator) called with an iterator not referring to this vector");
----------------
var-const wrote:
> ldionne wrote:
> > A part of me would be tempted to leave those assertions in place like this, just for documentation:
> > 
> > ```
> > _LIBCPP_LEGACY_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(std::addressof(__position)) == this, "vector::erase(iterator) called with an iterator not referring to this vector");
> > ```
> > 
> > This would expand to nothing, but it would document what kind of checking we used to do with the debug mode and would inform what we might want to check in the future, when we get there. WDYT?
> I thought about this, but was leaning towards removing those. We still have the effectively-disabled tests that check these assertions, and we can refer to the delta in this patch as well. But I'm definitely open to restoring these.
Ok, sold.


================
Comment at: libcxx/utils/gdb/libcxx/printers.py:994
             "__cxx1998",
-            "__debug",
             "__7",
----------------
I don't think this is correct -- this apparently pertains to libstdcxx types (per the comment above).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153672