[libcxx-commits] [PATCH] D118940: [libc++] Fix std::__debug_less in c++17.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

As philnik said, please revert all the extraneous changes. (And please ignore clang-format's noise going forward. We'd disable clang-format entirely if we could. This is an ongoing pain point for libc++, over and over and over.)

IIUC, the only intentional functional change here is to change `_LIBCPP_CONSTEXPR_AFTER_CXX17` into `_LIBCPP_CONSTEXPR_AFTER_CXX11`, is that right? That certainly seems like a good idea to me. (My mantra there is that //internal details should always use the most aggressive constexpr possible.// That way, if we ever drop support for C++03 and C++11 modes, the amount of stuff can be search-and-replaced to plain `constexpr` immediately is maximized.)

Contrariwise, your changes to `_LIBCPP_DEBUG` seem extraneous and/or wrong; I don't think you should touch anything in this PR that isn't related to the bug you're trying to fix. (And if we //were// going to touch anything in that area, I'd suggest that we remove the `#if`s around `__debug_less`: I see no reason ever to if-out its class definition, even if it's sometimes unused.)



================
Comment at: libcxx/include/__algorithm/comp_ref_type.h:67
   // debugging wrapper that stores a reference.
-#ifndef _LIBCPP_DEBUG
+#if _LIBCPP_DEBUG_LEVEL < 1
   typedef _Comp& type;
----------------
This is definitely wrong, because `_LIBCPP_DEBUG` might not be defined at all, in which case this change will trigger `-Wundef`. Please leave `_LIBCPP_DEBUG` alone unless it's related to your patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118940