Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CombinationsSequence: underestimatedCount == count #220

Merged

Conversation

dabrahams
Copy link
Contributor

Not doing this makes it needlessly costly to create an Array of the results.
It's a reasonable incremental improvement until you get around to #219, which would handle this automatically.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

Not doing this makes it needlessly costly to create an Array of the
results.

/// The total number of combinations.
@inlinable
public var underestimatedCount: Int { count }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this conflict with the complexity requirement of Sequence.underestimatedCount?

  /// - Complexity: O(1), except if the sequence also conforms to `Collection`.
  ///   In this case, see the documentation of `Collection.underestimatedCount`.
  var underestimatedCount: Int { get }

The requirement is relaxed for non-random access collections, so not a problem for #219.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, indeed. What is the complexity of your count? If it's log(N) IMO this is acceptable; complexity requirements are somewhat fungible (e.g. first on a lazy filtered collection is arguably not O(1)). Otherwise, I suppose not.

Oh, but they totally broke the complexity bound of underestimatedCount. It was O(n) before ea49459b7594. What a debacle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that O(1) constraint is meaningless as outlined here. You should ignore it.

@natecook1000
Copy link
Member

@swift-ci Please test

Co-authored-by: Dave Abrahams <[email protected]>
@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000 natecook1000 merged commit 9dd88e5 into apple:main Jan 20, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants