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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/CMakeLists.txt:59
 option(LIBCXX_ENABLE_SHARED "Build libc++ as a shared library." ON)
 option(LIBCXX_ENABLE_STATIC "Build libc++ as a static library." ON)
 option(LIBCXX_ENABLE_FILESYSTEM
----------------
ldionne wrote:
> Not attached: I think we need to modify the CI setup to remove the debug mode job (and the associated CMake cache).
Thanks!


================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:41
 
-#ifndef _LIBCPP_ENABLE_DEBUG_MODE
-
----------------
ldionne wrote:
> This raises an interesting question -- how do we make sure not to forget to come back and re-evaluate what we should do here in the future? I think we should leave some kind of `TODO(debug-mode)` behind or something like that.
I thought to simply use the delta in this patch, but adding a note SGTM.


================
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");
----------------
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.


================
Comment at: libcxx/utils/libcxx/test/features.py:310
     "_LIBCPP_HAS_NO_UNICODE": "libcpp-has-no-unicode",
-    "_LIBCPP_ENABLE_DEBUG_MODE": "libcpp-has-debug-mode",
 }
----------------
ldionne wrote:
> You should make a pass for the tests that use `libcpp-has-debug-mode` -- I see a few where stuff like `// XFAIL: libcpp-has-debug-mode` can be removed.
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153672