This has been broken out of D104170 since it should be merged whether or
not we go ahead with the module map changes.
Details
- Reviewers
ldionne zoecarver Mordante • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rG332da1c28356: [libcxx][iwyu] ensures we IWYU as prep for modules
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__memory/allocator_traits.h | ||
---|---|---|
360–368 | All the #include changes look fine to me, but I question the wisdom of(*) splitting up the specializations of __is_default_allocator. Generally for a type-trait we want to keep the "true" and "false" branches together in the same file. template <class _Tp> class allocator; right above line 360. (* yes, this is understatement) |
updates a lot of that were lacking explicit header inclusions (see most recent failures in D104170)
Include changes LGTM, but I don't understand the change to __function_like.
libcxx/include/__function_like.h | ||
---|---|---|
40 ↗ | (On Diff #351680) | Why is this change necessary? |
libcxx/include/__function_like.h | ||
---|---|---|
40 ↗ | (On Diff #351680) | The modules build seems to think that types derived from __function_like still have addresses. This sent me round the twist trying to figure out why, and I eventually explicitly deleted the operators in the derived types. I don't want anyone to need to go through this again, so I think it's best we delete the operator from the base type so people realise they need to add it to the derived type. |
libcxx/include/__function_like.h | ||
---|---|---|
40 ↗ | (On Diff #351680) | This explains the many changes in __iterator/advance.h etc. |
libcxx/include/__function_like.h | ||
---|---|---|
40 ↗ | (On Diff #351680) |
Sure, I can move it into D104170 patch, since it's directly driven by that patch.
It would be really beneficial if you bothered to understand design decisions instead of wrongly asserting that something isn't necessary ad infinitum. I'll spell it out for you, even though it's documented directly above this type's definition. Everything derived from this type is supposed to have the properties of a function. Functions cannot be default constructed. Functions cannot be moved. Functions cannot be copied. Standard functions are not allowed to have their addresses taken. It's extremely tempting to use functions that are derived from __function_like as higher-order functions. Hyrum's law dictates that users will eventually treat this as allowed if we don't prevent these things from happening. There was a deliberate decision to not make them function objects in the standard, and I do not want users to start treating them as if they are. |
No opinion on what patch the operator change goes in. Thanks for explaining the rational.
Send it :)
LGTM but can you please move the operator& changes to the other patch? Also, I think this does reduce the usefulness of __function_like by quite a bit, cause now we have to think about adding a declaration in the function objects *and* derive from __function_like. I think we should fix the underlying modules bug (seriously, modules shouldn't have anything to do with this!) and more operator& back into __function_like.
All the #include changes look fine to me, but I question the wisdom of(*) splitting up the specializations of __is_default_allocator. Generally for a type-trait we want to keep the "true" and "false" branches together in the same file.
So, please don't make this diff. Either leave it as-is, or, if you really can't get the modules build building without a diff here, make the diff to add
right above line 360.
(* yes, this is understatement)