This removes a few dependencies on <iterator> which removes the dependency cycle between iterator -> variant -> array -> iterator and iterator -> algorithm -> iterator and iterator -> memory -> algorithm -> iterator so that we can use variant in common_iterator.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
Comment Actions
A simpler way to say the words in the title: this removes <algorithm>'s dependency on both <memory> and <iterator>. (<memory> still depends on <iterator>.)
| libcxx/include/__algorithm/equal.h | ||
|---|---|---|
| 17 | a-z plz | |
| libcxx/include/__algorithm/is_permutation.h | ||
| 18 | a-z plz (and throughout, anywhere I missed) | |
| libcxx/include/__functional/function.h | ||
| 24 | a-z plz | |
| libcxx/include/algorithm | ||
| 654–656 | This line is duplicated (see below in a-z order) | |
| libcxx/include/array | ||
| 116 ↗ | (On Diff #357059) | a-z plz |
| libcxx/include/functional | ||
| 518 ↗ | (On Diff #357059) | As @EricWF and @ldionne (and I) have said, removing user-visible inclusions really ought to be done in separate commits and advertised. If we're breaking #include <functional> std::unique_ptr<int> p; then that needs to be advertised somehow before we do it. Or discussed. Or something. (I mean, we just keep churning these headers — I'm sure we've broken many things already — but expect to keep hearing the same old squeaky wheels until the churn stops.) |
Comment Actions
Splitting stuff out of <memory> is good, but please don't remove any header inclusions just yet. A cross-sectional plan (that I'm working out) needs to be put into place before we can delete our inclusions.
EDIT: if we need to remove something, we'll need to precisely document it in our release notes (including why we needed to push the deletion ahead of schedule), and give users the chance to respond to this change.
Comment Actions
EDIT: if we need to remove something, we'll need to precisely document it in our release notes (including why we needed to push the deletion ahead of schedule), and give users the chance to respond to this change.
If we want to use variant in common_iterator (which I am neutral on at best) we do need to remove something. We can do one of the following:
- Remove <iterator> and <algorithm> from <array>
- Remove <memory> and <iterator> from <algorithm> (current).
Putting this here preemptively: based on recent discussions, I'm going to do the first one, as that seems to be lower-impact. I'm also going to keep all the changes to internal headers, but make sure that external headers still provide the same includes (except for <array>).
Comment Actions
Zoe and I just spoke.
He's going to turn this into a NFC where we don't remove includes from the "public" headers, but still split stuff out of <memory>.
In a follow-up patch, we can try to break the cycle between <variant> and <array> by using a raw C array in variant's implementation and removing #include <array> from <variant>. Yes, that will be a breaking change. But I don't think we need a formal release note and discussion for just that one - we've been making small breaking changes such as this without even noticing it for years. While we should have a discussion if we're going to do something major and systematic like in D104980, we shouldn't prevent ourselves from making progress for something as simple as this.
This LGTM once the patch is updated to be a NFC (giving thumbs up proactively because we're not in the same time zone and I want this to be unblocked).
Comment Actions
I may pick this patch up again later, but for now, I am not going to spend hours debugging why clang is crashing. This patch is not blocking anything.
a-z plz