[libcxx-commits] [PATCH] D119769: [libc++] Move everything related solely to _LIBCPP_ASSERT to its own file

Tue Feb 15 09:11:13 PST 2022

ldionne marked 6 inline comments as done.
ldionne added inline comments.

================
Comment at: libcxx/include/CMakeLists.txt:102
   __algorithm/upper_bound.h
+  __assert
   __availability
----------------
Quuxplusone wrote:
> philnik wrote:
> > Quuxplusone wrote:
> > > Are we at all worried about name conflicts? (Or stylistically about adding new top-level detail headers?) Should this be something like `__utility/libcpp_assert.h` instead?
> > > OTOH, maybe we keep it like this right now, and then at some point in the future we move `__assert` to `__foo/libcpp_assert.h` and `__debug` to `__foo/debug_iterators.h` in the same commit, for some value of `__foo` on which I'm sure we'll disagree. :)
> > I'm definitely in favor of moving that stuff into some directory. Maybe `__libcpp/`? That would most likely not clash with any new C++ headers and we could leave the `libcpp_` prefix out.
> > and we could leave the `libcpp_` prefix out.
> 
> I don't want to get into a situation where `<assert.h>` means one thing and `"./assert.h"` means a different thing, so the prefix would need to stay even in that case. (Likewise, if we ever granularize `<string>`, I'll oppose the creation of `__string/string.h`. But luckily in that case it'll be `__string/basic_string.h` so the issue won't arise.)
I agree that we should move those to a subdirectory, however I want to do that separately to give us ample time to bikeshed the name.

We already have a "pattern" for adding these detail headers right now: it's to add them at the top-level. So I'm following the trend in this patch for the time being.

================
Comment at: libcxx/include/__debug:27

-#if _LIBCPP_DEBUG_LEVEL == 0
+#if _LIBCPP_DEBUG_LEVEL < 2
 #   define _LIBCPP_DEBUG_ASSERT(x, m) ((void)0)
----------------
philnik wrote:
> Quuxplusone wrote:
> > This `<` allows people to set `-D_LIBCPP_DEBUG_LEVEL=-1` without triggering the `#error` on line 32.
> `_LIBCPP_DEBUG_LEVEL` should never be set directly. The public interface is `_LIBCPP_DEBUG`.
I agree with Arthur, I'll fix it. This is an easy way to ensure that users don't define things they shouldn't define.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119769