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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.


================
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)
----------------
ldionne wrote:
> 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)
> ```
Thanks both for the feedback. I was doubting between these two options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145798