This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove the type_traits includes from limits and new
ClosedPublic

Authored by iana on Jun 17 2023, 6:57 PM.

Details

Summary

type_traits is currently unable to include __type_traits/noexcept_move_assign_container.h, because it would cause several include cycles.

type_traits -> type_traits/noexcept_move_assign_container.h -> memory/allocator_traits.h -> __memory/construct_at.h -> new -> exception -> type_traits

type_traits -> type_traits/noexcept_move_assign_container.h -> memory/allocator_traits.h -> __memory/construct_at.h -> new -> type_traits

type_traits -> type_traits/noexcept_move_assign_container.h -> memory/allocator_traits.h -> limits -> type_traits

This is a problem for clang modules after the std mega module is broken up (D144322), because it becomes a module cycle which is a hard error.

Unconditionally remove the type_traits includes from limits and new in all versions, and also remove the exception include from new. (These are already removed in C++23.)

Diff Detail

Event Timeline

iana created this revision.Jun 17 2023, 6:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2023, 6:57 PM
Herald added a subscriber: ributzka. · View Herald Transcript
iana requested review of this revision.Jun 17 2023, 6:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2023, 6:57 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante requested changes to this revision.Jun 18 2023, 7:55 AM

I've no principle objection against this patch. However this patch feels incomplete since it does not update the transitive includes. I much rather move the patch out of the stack and make it a stand-alone patch; then we can see how it affects the transitive includes.

This revision now requires changes to proceed.Jun 18 2023, 7:55 AM
iana added a comment.Jun 18 2023, 7:28 PM

I've no principle objection against this patch. However this patch feels incomplete since it does not update the transitive includes. I much rather move the patch out of the stack and make it a stand-alone patch; then we can see how it affects the transitive includes.

I moved it to the front of the stack, and now the transitive include changes show up.

iana updated this revision to Diff 532503.Jun 18 2023, 7:29 PM

Move this change to the front of the stack so that the transitive include modifications show up.

ldionne accepted this revision as: ldionne.Jun 21 2023, 12:56 PM

I'm fine with this since it's helpful to make progress on fixing our Clang modules support. It'll hurt a bit, as always, but <new> and <limits> are not the most widely included headers anyways so this might not have such a big impact. In any case, better land this now than closer to the branch date.

Leaving final approval to @Mordante since he had requested changes.

Mordante accepted this revision.Jun 24 2023, 6:02 AM

I'm fine with this since it's helpful to make progress on fixing our Clang modules support. It'll hurt a bit, as always, but <new> and <limits> are not the most widely included headers anyways so this might not have such a big impact. In any case, better land this now than closer to the branch date.

+1 to landing this sooner than later.

LGTM!

This revision is now accepted and ready to land.Jun 24 2023, 6:02 AM
This revision was automatically updated to reflect the committed changes.