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

Mon Jun 26 13:02:52 PDT 2023

var-const created this revision.
Herald added a subscriber: arichardson.
Herald added a project: All.
var-const updated this revision to Diff 534135.
var-const added a comment.
var-const updated this revision to Diff 534628.
ldionne published this revision for review.
Herald added a reviewer: jdoerfert.
Herald added subscribers: libcxx-commits, jplehr, sstefan1.
Herald added a project: libc++.
Herald added a reviewer: libc++.

Compilation fixes.

var-const added a comment.

CI fix.

================
Comment at: libcxx/CMakeLists.txt:57
    default when compiling user code. Note that assertions can be enabled or disabled
    by users in their own code regardless of this option." OFF)
 option(LIBCXX_ENABLE_SHARED "Build libc++ as a shared library." ON)
----------------
Not attached: We should add a release note.

================
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
----------------
Not attached: I think we need to modify the CI setup to remove the debug mode job (and the associated CMake cache).

================
Comment at: libcxx/include/__algorithm/unwrap_iter.h:41

-#ifndef _LIBCPP_ENABLE_DEBUG_MODE
-
----------------
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.

================
Comment at: libcxx/include/__format/buffer.h:256
     (same_as<_OutIt, _CharT*>
-#ifndef _LIBCPP_ENABLE_DEBUG_MODE
      || same_as<_OutIt, __wrap_iter<_CharT*>>
----------------
Same here -- those seem like things we'd want to do with our ownership checking and bounded iterators as well?

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

================
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",
 }
----------------
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.

See https://discourse.llvm.org/t/rfc-removing-the-legacy-debug-mode-from-libc/71026

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153672

Files:
  libcxx/CMakeLists.txt
  libcxx/include/CMakeLists.txt
  libcxx/include/__algorithm/comp_ref_type.h
  libcxx/include/__algorithm/nth_element.h
  libcxx/include/__algorithm/partial_sort.h
  libcxx/include/__algorithm/shuffle.h
  libcxx/include/__algorithm/sort.h
  libcxx/include/__algorithm/three_way_comp_ref_type.h
  libcxx/include/__algorithm/unwrap_iter.h
  libcxx/include/__assert
  libcxx/include/__config_site.in
  libcxx/include/__debug
  libcxx/include/__format/buffer.h
  libcxx/include/__format/format_functions.h
  libcxx/include/__format/formatter_bool.h
  libcxx/include/__format/parser_std_format_spec.h
  libcxx/include/__hash_table
  libcxx/include/__iterator/wrap_iter.h
  libcxx/include/__ranges/filter_view.h
  libcxx/include/__tree
  libcxx/include/algorithm
  libcxx/include/charconv
  libcxx/include/functional
  libcxx/include/iterator
  libcxx/include/list
  libcxx/include/locale
  libcxx/include/module.modulemap.in
  libcxx/include/span
  libcxx/include/string
  libcxx/include/unordered_map
  libcxx/include/unordered_set
  libcxx/include/vector
  libcxx/src/CMakeLists.txt
  libcxx/src/debug.cpp
  libcxx/src/include/ryu/ryu.h
  libcxx/test/libcxx/algorithms/alg.sorting/alg.heap.operations/make.heap/complexity.pass.cpp
  libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/sort.heap/complexity.pass.cpp
  libcxx/test/std/algorithms/alg.sorting/alg.heap.operations/sort.heap/ranges_sort_heap.pass.cpp
  libcxx/test/std/algorithms/alg.sorting/alg.merge/inplace_merge_comp.pass.cpp
  libcxx/test/std/algorithms/alg.sorting/alg.three.way/lexicographical_compare_three_way_comp.pass.cpp
  libcxx/test/support/container_debug_tests.h
  libcxx/utils/data/ignore_format.txt
  libcxx/utils/gdb/libcxx/printers.py
  libcxx/utils/generate_iwyu_mapping.py
  libcxx/utils/libcxx/test/features.py
  libcxx/utils/libcxx/test/header_information.py

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D153672.534628.patch
Type: text/x-patch
Size: 164181 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230626/5a5f9316/attachment-0001.bin>