[libcxx-commits] [PATCH] D106763: [libc++][RFC] Disable incomplete library features.

Tue Jul 27 02:06:22 PDT 2021

Mordante marked an inline comment as done.
Mordante added a comment.

Thanks for your feedback, this is exactly the kind of feedback I hoped to get for this RFC!

> I actually gave more thought to this, and I think what we want is:
>
> 1. Add a macro in `__config_site` like `_LIBCPP_HAS_NO_INCOMPLETE_FEATURES`.

Initially I went this path, but I wasn't too happy with it. I split it in one macro for format and one macro for ranges. This means there will be `libcpp-has-no-incomplete-ranges` and `libcpp-has-no-incomplete-format`. I prefer two different names. Once one feature is completed it's a lot easier to grep whether all occurrences have been removed.

> 2. Add a CMake option `LIBCXX_ENABLE_INCOMPLETE_FEATURES` that sets this macro in `__config_site`. By default, that option should be set to `ON` so that we enable incomplete features by default.
> 3. Add a macro in `features.py` like: `'_LIBCPP_HAS_NO_INCOMPLETE_FEATURES': 'libcpp-has-no-incomplete-features'`, and mark the relevant tests as `UNSUPPORTED`.
> 4. Set `set(LIBCXX_ENABLE_INCOMPLETE_FEATURES OFF CACHE BOOL "")` in `Apple.cmake`.

I prefer to have some Apple CI bots to override this default. That way we can validate whether the feature would work on Apple. Otherwise when we flip the switch we get all Apple build issues at the same time. I was thinking about using `MacOS x86_64` and `MacOS arm64`. WDYT?

> That way, incomplete features are always enabled by default (i.e. the current state of things). Enabling them does not require any specific action. However, when we want to ship libc++ (as a vendor or as part of the LLVM distribution), we can disable the incomplete features when configuring CMake.
>
> The benefits of doing it that way is that it's better integrated with the rest of the code -- that's how we disable all kinds of other features. Also, it's not clear to me that we want to give users the ability to override this option. It's not like using something in `std::experimental`, it's really more like using something that isn't fully baked yet.
>
> If you agree and we go in that direction, we should also move the documentation for the option to `BuildingLibcxx.rst`, which is aimed at vendors instead of end-users.

I like this direction, it refines the general direction of the original proposal.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106763