This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Disables transitive includes in library.
ClosedPublic

Authored by Mordante on Mar 10 2023, 8:34 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGc416234c3841: [libc++] Disables transitive includes in library.
Summary

I noticed this wile investigating https://llvm.org/PR61314

Diff Detail

Event Timeline

Mordante created this revision.Mar 10 2023, 8:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2023, 8:34 AM
Mordante requested review of this revision.Mar 10 2023, 8:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2023, 8:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik added inline comments.
libcxx/include/__config
278–279 ↗(On Diff #504158)

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

Mordante added inline comments.
libcxx/include/__config
278–279 ↗(On Diff #504158)

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.

ldionne accepted this revision.Mar 14 2023, 8:43 AM

LGTM once set within CMake

libcxx/include/__config
278–279 ↗(On Diff #504158)

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)
This revision is now accepted and ready to land.Mar 14 2023, 8:43 AM
Mordante marked 2 inline comments as done.Mar 14 2023, 10:36 AM
Mordante added inline comments.
libcxx/include/__config
278–279 ↗(On Diff #504158)

Thanks both for the feedback. I was doubting between these two options.

Mordante updated this revision to Diff 505164.Mar 14 2023, 10:36 AM
Mordante marked an inline comment as done.

Addresses review comments. Give it a CI run before landing.

This revision was landed with ongoing or failed builds.Mar 15 2023, 10:01 AM
This revision was automatically updated to reflect the committed changes.