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

Minor code efficiency issue, use of LineIsMultiLine() #1125

Closed
msftrncs opened this issue Oct 25, 2019 · 3 comments · Fixed by #2047
Closed

Minor code efficiency issue, use of LineIsMultiLine() #1125

msftrncs opened this issue Oct 25, 2019 · 3 comments · Fixed by #2047
Assignees

Comments

@msftrncs
Copy link
Collaborator

While working on #1108, while proposing to have InsertLineAbove() utilize GetBeginningOfLinePos() since it performs the exact same loop backward through the buffer searching for a \n, I was almost going to leave the original reference to LineIsMultiLine() in InsertLineAbove(), though it also occurs in GetBeginningOfLinePos().

However when I look at the code for LineIsMultiLine() it is itself a search through the buffer for a \n. Statistically, if LineIsMultiLine() finds a \n then there is now going to be Ta + Tb time spent searching for \n instead of just Tb time spent, where Ta is time spent to find the first \n in the buffer and Tb is time spent to find a \n under the specific scenario of the function. In the case where no \n is found, LineIsMultiLine() will touch every character in the buffer to confirm that, where as in the specific scenario of the function will result in a variable number characters being checked, from 0 through all of them, depending on the scenario.

Is there a reason for this approach?

Is there a downside to removing the use of LineIsMultiLine() when making changes to such functions?

@springcomp
Copy link
Contributor

I originally introduced LineIsMultiline when I started working in improving vi-mode to more closely work with multiline buffers. I did that so that I could tailor the code in generic methods that are also used in the Emacs-mode because I did not want to change the behaviours of these functions.

Most of the time though, I find it best to write algorithms that natively work irrespective of the fact that the buffer contains a single line or multiple lines, without resorting to using LineIsMultiline.

At the end of the day, the unit tests will tell you if you can shortcut and improve the code.

@ghost
Copy link

ghost commented Feb 23, 2021

🎉 This issue was addressed in 2047, which has now been successfully released in v2.2.0-beta2. 🎉

@ghost
Copy link

ghost commented Feb 23, 2021

🎉 v2.2.0-beta2 has been released which incorporates this pull request. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants