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

Thu Feb 3 14:34:37 PST 2022

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

That's correct -- supporting this for C++17 was the only change I care about.

> 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 lean towards not making unrelated changes in patches, but I wasn't sure how libc++ reviewers felt about this, so I made those changes after review comments. They're reverted now. How about I land just the part I care about in this commit, and do the rest in a followup?

- Replacing uses of `_LIBCPP_DEBUG` with checking `_LIBCPP_DEBUG_LEVEL` levels instead seems limited to just this file
- Replacing uses of `_LIBCPP_INLINE_VISIBILITY` with `_LIBCPP_HIDE_FROM_ABI` seems a lot larger (289 files affected, if I'm grepping correctly), so I'd prefer not to do the whole cleanup (although it's not complex -- it's just a sed command), but I could do it for this file.

================
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;
----------------
philnik wrote:
> 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.
Yep, https://github.com/llvm/llvm-project/blob/c3c1c5c6953fcf9320a0cae5121ce55839169790/libcxx/include/__config#L911

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118940