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

Mon Jul 3 06:36:52 PDT 2023

ldionne added inline comments.

================
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
----------------
hans wrote:
> ldionne wrote:
> > 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.
> This caused a few of Chromium's tests to fail.
> 
> Since we set `_LIBCPP_ENABLE_ASSERTIONS` but not `_LIBCPP_ENABLE_DEBUG_MODE`, this made `std::tree<>::erase` `O(n)` instead of `O(log n)`, causing a few of our tests to time out. (crbug.com/1459922)
> 
> It seems this kind of check is too expensive for the regular assert mode. Could this check be disabled for now, or is there some alternative "expensive checks" mode it could be part of?
Interesting -- yes I think this is clearly a debug mode assertion only, we only happen to be in the middle of categorizing those so the current state of the tree doesn't represent what we want to ship in the next release. @var-const I think we could just comment out this assertion if it's causing problems until we categorize it as a debug mode assertion.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153672