[vim/vim] Refactor os_win32.c to remove calls to STRLEN() (PR #17462)

John Marriott

unread,

Jun 6, 2025, 7:37:10 PM6/6/25

to vim/vim, Subscribed

In mch_init_g():
-> refactor to remove calls to STRLEN().
-> use vim_strnsave() instead of vim_strsave().
-> set a flag if vimrun_path is stored in allocated memory so it can be freed at exit.
In mch_exit() free vimrun_path if it was stored in allocated memory.
In fname_case() make a small optimisation by measuring the length of name only if needed.
In copy_extattr() make a small optimisation by replacing call to STRCAT() with STRCPY().

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

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

Commit Summary

123e97d Refactor os_win32.c to remove calls to STRLEN()

File Changes

(1 file)

M src/os_win32.c (96)

Patch Links:

—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.

Copilot

unread,

Jun 7, 2025, 12:13:40 PM6/7/25

to vim/vim, Subscribed

@Copilot commented on this pull request.

Pull Request Overview

Refactors Windows-specific runtime code to remove direct STRLEN()/STRCAT() calls, use length-aware string functions, and track allocated memory for the vimrun invocation path.

Use string_T and vim_strnsave() to store and measure vimrun_path, tracking allocation for proper freeing.
Replace STRCAT() with STRCPY() offsets and avoid redundant STRLEN() calls in fname_case() for small performance gains.
Free allocated vimrun_path in mch_exit() and adjust logic for long paths and quoted paths.

Comments suppressed due to low confidence (2)

src/os_win32.c:168

Add a brief comment explaining the purpose of vimrun_path_allocated, e.g., "// Indicates whether vimrun_path.string was heap-allocated and needs to be freed."

static int vimrun_path_allocated = FALSE;

src/os_win32.c:3029

This branch handles very long executable paths; consider adding a test to cover the case where exe_pathlen + 10 exceeds the buffer size to verify fallback behavior.

if (exe_pathlen + 10 >= sizeof(vimrun_location))

In src/os_win32.c:

>      if (gui.in_use || gui.starting)
+    {
+	if (vimrun_path_allocated)
+	    vim_free(vimrun_path.string);
 	mch_exit_g(r);
+    }
     else
 	mch_exit_c(r);

When building as VIMDLL, the allocated vimrun_path is only freed in the if (gui.in_use || gui.starting) branch. If vimrun_path_allocated is true but that condition is false, vimrun_path won’t be freed, causing a leak. Consider moving the free call outside or adding it to the else branch.

⬇️ Suggested change

-    if (gui.in_use || gui.starting)
-    {
-	if (vimrun_path_allocated)
-	    vim_free(vimrun_path.string);
-	mch_exit_g(r);
-    }
-    else
-	mch_exit_c(r);
+    if (vimrun_path_allocated)
+        vim_free(vimrun_path.string);
+
+    if (gui.in_use || gui.starting)
+        mch_exit_g(r);
+    else
+        mch_exit_c(r);

In src/os_win32.c:

> +
+	    if (len > 0 || namelen >= STRLEN(q))
+		vim_strncpy(name, q, (len > 0) ? len - 1 : namelen);

[nitpick] You call STRLEN(q) twice (once in the condition, once in vim_strncpy ternary). Consider caching STRLEN(q) in a local variable to improve readability and avoid repeated computation.

⬇️ Suggested change

-
-	    if (len > 0 || namelen >= STRLEN(q))
-		vim_strncpy(name, q, (len > 0) ? len - 1 : namelen);
+	    size_t  q_len = STRLEN(q);
+
+	    if (len > 0 || namelen >= q_len)
+		vim_strncpy(name, q, (len > 0) ? len - 1 : q_len);

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

John Marriott

unread,

Jun 7, 2025, 7:37:13 PM6/7/25

to vim/vim, Push

@basilisk0315 pushed 1 commit.

b26007e Changes based on AI feedback

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Jun 8, 2025, 10:05:37 AM6/8/25

to vim/vim, Subscribed

chrisbra left a comment (vim/vim#17462)

thanks

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Jun 8, 2025, 10:09:50 AM6/8/25

to vim/vim, Subscribed

Closed #17462 via 9cb27a5.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Reply all

Reply to author

Forward