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

Thu Feb 3 14:27:49 PST 2022

philnik added a comment.

In D118940#3295112 <https://reviews.llvm.org/D118940#3295112>, @Quuxplusone wrote:

> 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.)

I don't particularly care if we remove the `_LIBCPP_DEBUG`s around `__debug_less`, but I think they can stay in.

================
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;
----------------
Quuxplusone wrote:
> 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.
If `_LIBCPP_DEBUG` isn't defined, `_LIBCPP_DEBUG_LEVEL` is 0. This should probabl just be `== 0`, but it's not wrong.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118940