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

Allow empty ColList #1588

Merged
merged 1 commit into from
Aug 15, 2024
Merged

Allow empty ColList #1588

merged 1 commit into from
Aug 15, 2024

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Aug 14, 2024

Description of Changes

Fix the bug found in #1585 by allowing ColLists to be empty, as we actually try to use the type as a possibly-empty list in some places (which is why the tests failed in #1585).

API and ABI breaking changes

None

Expected complexity level and risk

2, some unsafe related code affected, but there's less unsafe code now.

Testing

  • Tests were extended to test the empty aspect and the new functionality.
  • All the ColList proptests were run in miri.

Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

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

Looks good. Nice to be able to use collect.

I was sketching an implementation of sort as well, since I need it for finding indexes for unique constraints, but it's a little too much added code to do with suggestions.
I'll open a followup PR once this is merged.

gefjon
gefjon previously requested changes Aug 14, 2024
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Bump Rust to 1.80.1 as this turned out to be required due to compiler bugs on 1.78.

I would like a more detailed description than this, and would also like to know why bumping to 1.79 is insufficient. I would also strongly prefer the version bump happen in a separate PR, rather than sneaking it in alongside another change.

@Centril Centril force-pushed the centril/col-list-can-be-empty branch 2 times, most recently from e7a1cd3 to e595ce2 Compare August 15, 2024 11:04
@Centril Centril force-pushed the centril/col-list-can-be-empty branch 2 times, most recently from 69dee6c to d738f9f Compare August 15, 2024 11:37
@Centril Centril changed the title Allow empty ColList + Bump Rust to 1.80.1 Allow empty ColList Aug 15, 2024
@Centril
Copy link
Contributor Author

Centril commented Aug 15, 2024

I would like a more detailed description than this, and would also like to know why bumping to 1.79 is insufficient. I would also strongly prefer the version bump happen in a separate PR, rather than sneaking it in alongside another change.

It was something at some point which caused complaints about #![recursion_limit = "256, 512, ..."] which went away after I bumped the version, but it seems like it works now without a bump, so I removed that from the PR and saved the bump to a different branch.

@Centril Centril dismissed gefjon’s stale review August 15, 2024 11:56

Rust bump removed from PR.

@Centril Centril added this pull request to the merge queue Aug 15, 2024
Merged via the queue into master with commit 6a08674 Aug 15, 2024
9 checks passed
@Centril Centril deleted the centril/col-list-can-be-empty branch August 15, 2024 17:04
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