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

Avoid cloning the whole paragraph content just for rendering #9739

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

mo8it
Copy link
Contributor

@mo8it mo8it commented Feb 27, 2024

No corresponding issue, just found it odd while reading. Feel free to reject it, but I think that cloning the whole content of a paragraph just to render it can be significant. This is not just one allocation, these are multiple allocations because Text is a vector of vectors of possibly owned strings.

pascalkuthe
pascalkuthe previously approved these changes Feb 27, 2024
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

I want to get rid of the utilities in helix-tui anyway and use the wrapping from helix-core anyway. This code doesn't get much attention. But I would be fine with this

@mo8it
Copy link
Contributor Author

mo8it commented Feb 27, 2024

Fixed the tests. I couldn't run them locally because of #9710

@the-mikedavis the-mikedavis added the S-waiting-on-review Status: Awaiting review from a maintainer. label Feb 27, 2024
@pascalkuthe pascalkuthe merged commit 00653c7 into helix-editor:master Feb 27, 2024
6 checks passed
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
…ditor#9739)

* Avoid cloning the whole paragraph content just for rendering

* Fix tests
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
…ditor#9739)

* Avoid cloning the whole paragraph content just for rendering

* Fix tests
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
…ditor#9739)

* Avoid cloning the whole paragraph content just for rendering

* Fix tests
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
…ditor#9739)

* Avoid cloning the whole paragraph content just for rendering

* Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants