[libcxx-commits] [PATCH] D145798: [libc++] Disables transitive includes in library.

Tue Mar 14 08:43:33 PDT 2023

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM once set within CMake

================
Comment at: libcxx/include/__config:278-279
+// it easier to upgrade the library to a newer language standard without build
+// errors. Since users are allowed to define _LIBCPP_REMOVE_TRANSITIVE_INCLUDES
+// it can't be set as a compiler flag in CMake.
+#  if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && defined(_LIBCPP_BUILDING_LIBRARY)
----------------
Mordante wrote:
> philnik wrote:
> > I don't understand. Why would this make a difference whether a user sets it or not? We should be able to set this when building the library, just like we set `_LIBCPP_BUILDING_LIBRARY` (and I think we should set it in CMake).
> I'm not sure whether that would break some unexpected cases where vendors do things we don't expect.
> For example I know Chromium uses this option and they build their own libc++.
> If @ldionne agrees with you I'm happy to move it to the CMakeLists,txt directly below where we define 
> `_LIBCPP_BUILDING_LIBRARY`.
Hmm yes, I think this would be the better approach:

```
target_compile_definitions(${target} PRIVATE -D_LIBCPP_BUILDING_LIBRARY)

// Make sure the library can be build without transitive includes. This makes
// it easier to upgrade the library to a newer language standard without build
// errors.
target_compile_definitions(${target} PRIVATE -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES)
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145798