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

Remove some superlinear complexity #76

Merged
merged 4 commits into from
Jun 27, 2018
Merged

Conversation

brson
Copy link
Collaborator

@brson brson commented Jun 27, 2018

Both unescape and ltrim were doing exponential work based on the number of matches in the input, by calling remove repeatedly, thus re-copying the same tail of the vector.

Surprisingly, I don't see a better safe tool for this than rotate_left offhand, which is a recently-stabilized API (and still isn't really the right tool), so I wrote some unsafe code, and only tested it lightly.

cc @SSJohns

Also make rtrim more idiomatic.
The unsafety is unfortunate but I don't see a safe tool
in std for doing this quickly.
Copy link
Owner

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

LGTM. Unsafe code seems fairly foolproof. (famous last words.)

@brson
Copy link
Collaborator Author

brson commented Jun 27, 2018

Agh, copy_from is a 1.26 API too. I'll update it with an older function.

@kivikakk
Copy link
Owner

Are you depending on an earlier version? I figure targeting current stable (now 1.27) is reasonable given Rust's stable backwards compatibility.

@brson
Copy link
Collaborator Author

brson commented Jun 27, 2018

@kivikakk No I'm not, but comrak works with toolchains at least as far back as 1.23, and this doesn't seem worth breaking compatibility over.

@brson
Copy link
Collaborator Author

brson commented Jun 27, 2018

Now it's using ptr::copy.

@kivikakk kivikakk merged commit 10dd0db into kivikakk:master Jun 27, 2018
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