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

Explore removing SmallVec from Event enum #614

Merged
merged 4 commits into from
Apr 10, 2022
Merged

Explore removing SmallVec from Event enum #614

merged 4 commits into from
Apr 10, 2022

Conversation

lopopolo
Copy link
Contributor

@lopopolo lopopolo commented Apr 10, 2022

This PR explores removing the dependency on smallvec and replacing it with Vec from std.

smallvec is a third party dependency that leaks into the public API. If applications that depend on rustyline wish to create custom key bindings, they must also depend on smallvec to construct Event::KeySeq.

This PR replaces smallvec, which is a complicated data structure with lots of unsafe code and several known security advisories, with a vocabulary type from std.

This has the effect of reducing the size of the Event enum.

Because Event is a public enum, changing the type of the field in Event::KeySeq is a breaking change. I've shown this change in the sequence of commits in this branch and the addition of a test.

Motivation of this PR is reducing dependencies.

@gwenn if you'd prefer, I'd be willing to try an alternate PR that conditionally enables smallvec with a Cargo feature. I think it looks easy enough to support.

@gwenn
Copy link
Collaborator

gwenn commented Apr 10, 2022

$ cargo tree
...
├── radix_trie v0.2.1
│   ├── endian-type v0.1.2
│   └── nibble_vec v0.1.0
│       └── smallvec v1.8.0
├── smallvec v1.8.0

@lopopolo
Copy link
Contributor Author

😅 I am hoping to follow up with additional PRs that make the radix_trie dependency optional as well.

@gwenn gwenn merged commit 01d01d8 into kkawakam:master Apr 10, 2022
@lopopolo lopopolo deleted the lopopolo/remove-small-vec branch April 10, 2022 15:58
@lopopolo
Copy link
Contributor Author

😅 I am hoping to follow up with additional PRs that make the radix_trie dependency optional as well.

I've followed up on this here: #615

lopopolo added a commit to artichoke/artichoke that referenced this pull request Aug 2, 2022
`rustyline` 10.0.0 includes a breaking change where `Editor::new` now
returns result. The REPL propagates this error as an unhandled readline
error, which will exit the REPL with an error message.

This is a handcrafted version of @dependabot's PR in #1999 to address
this breaking change.

`rustyline` 10.0.0 includes several commits (a few of which I authored)
that reduce the number of deps pulled in a bunch:

- kkawakam/rustyline#613
- kkawakam/rustyline#614
- kkawakam/rustyline#615
- kkawakam/rustyline#623
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.

2 participants