[vim/vim] Additional refactoring in ex_substitute() (PR #20447)

[John Marriott]

This PR does some minor refactoring in ex_substitute().

Including:
Store global variable old_sub and local variable sub as a string_T to keep track of and use their lengths.
Rename some variables to better reflect their meaning and use.
Move some variables closer to where they are used.
If !keeppatterns only replace the existing value of old_sub if the memory allocation succeeds.
Replace magic number 50 with local enumeration (EXTRA_SIZE).
Simplify allocation of new_start.string.

Cheers
John

---

You can view, comment on, or merge this pull request online at:

  https://github.com/vim/vim/pull/20447

Commit Summary

• 622354c Minor additional refactoring in ex_subsitute()
• e2e8fb9 Small changes
• 099c6a4 Small changes
• 99dfbbe Refine allocation of new_start.string

File Changes

(1 file)

• M src/ex_cmds.c (208)

Patch Links:

• https://github.com/vim/vim/pull/20447.patch
• https://github.com/vim/vim/pull/20447.diff

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/20447@github.com>

[h_east]

h-east left a comment (vim/vim#20447)

@basilisk0315
I don't believe that converting to string_T is always the right thing to do. As I've mentioned before, it should be limited to cases where it brings a significant performance improvement.

With the two goals you state —

1. performance improvement, and
2. better code readability —
   you could justify modifying virtually any code anywhere.

I'd rather not have to say this, but your changes have introduced a fair number of regressions, and this PR is no exception. Take sub.length: you compute it as cmd - p, which is one larger than STRLEN whenever a trailing delimiter is present — i.e. on the most common path, :s/foo/bar/. The single field this entire refactor is built around is simply wrong, and it silently propagates into old_sub.length. On top of that you left a no-op sub.length = sub.length; self-assignment in the code. Neither of these would have survived a careful read-through.

You need to spend more time on self code review. Please also run an AI (LLM) review before submitting.

As I said at the outset: going forward, please limit your PRs to cases where performance is significantly improved — for example, where the processing speed of the affected code becomes at least 1.03× faster than before.

---

Speaking as one of the maintainers, this is just my personal view. The final call is up to @chrisbra .

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/20447/c4650250308@github.com>

[zeertzjq]

zeertzjq left a comment (vim/vim#20447)

This PR mostly saves STRLEN calls inside vim_strsave() by using vim_strnsave() instead, which I doubt will lead to a significant performance improvement.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/20447/c4650641227@github.com>

[Christian Brabandt]

chrisbra left a comment (vim/vim#20447)

okay, then let's not do it then.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/20447/c4652662011@github.com>