[vim/vim] Use the return value of copy_option_part() (PR #19725)
John Marriott
This PR refactors some source files to use the return value of copy_option_part().
In addition:
In memline.c:
->In function recover_names():
->->Replace calls to vim_strsave() with vim_strnsave() for the literal strings.
->->Use a string_T to store local variable buf.
In bufwrite.c:
->In function buf_write(), move variable wp to where it is used.
In help.c:
->In function fix_help_buffer(), replace call to add_pathsep() with after_pathsep().
In optionstr.c:
->In function export_myvimdir():
->->Use a string_T to store local variable buf.
->->Replace call to add_pathsep() with after_pathsep().
In scriptfile.c:
->In function do_in_path():
->->Use a string_T to store local variable buf.
->->Measure the lengths of prefix and name once before the while loop.
->->Replace call to add_pathsep() with after_pathsep().*
->->Move some variables closer to where they are used.
In spellfile.c:
-> In function init_spellfile():
->->Use a string_T to store local variable buf.
Cheers
John
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/19725
Commit Summary
- cc79839 This PR refactors some source files to use the return value of copy_option_part()
- a685876 Adjust some types
File Changes
(10 files)
- M src/bufwrite.c (32)
- M src/change.c (5)
- M src/cindent.c (11)
- M src/help.c (14)
- M src/memline.c (48)
- M src/optionstr.c (18)
- M src/scriptfile.c (90)
- M src/spell.c (7)
- M src/spellfile.c (47)
- M src/viminfo.c (7)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19725)Oops, sorry about that.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
In src/memline.c:
> #endif
#if defined(UNIX) || defined(MSWIN)
// For Unix names starting with a dot are special. MS-Windows
// supports this too, on some file systems.
- names[1] = vim_strsave((char_u *)".*.sw?");
- names[2] = vim_strsave((char_u *)".sw?");
+ names[1] = vim_strnsave((char_u *)".*.sw?", STRLEN_LITERAL(".*.sw?$"));
typo?
In src/bufwrite.c:
> @@ -1568,10 +1577,17 @@ buf_write(
while (*dirp)
{
// Isolate one directory name and make the backup file name.
- (void)copy_option_part(&dirp, IObuff, IOSIZE, ",");
+#if defined(UNIX) || defined(MSWIN)
+ int IObufflen;
+
+ IObufflen =
+#else
+ (void)
+#endif
I suppose this block is to avoid unused warnings? This is really ugly, can we annotate the variables with UNUSED ? I am not sure if this really works, because we typically only use this for function arguments.
In src/cindent.c:
> if (what == COM_START)
{
STRCPY(lead_start, lead_end);
- lead_start_len = (int)STRLEN(lead_start);
+ lead_start_len = lead_end_len;
This threw me off a little. Is this really true, that lead_start_len here is the same as lead_end_len?
In src/cindent.c:
> start_off = off;
start_align = align;
}
else if (what == COM_MIDDLE)
{
STRCPY(lead_middle, lead_end);
- lead_middle_len = (int)STRLEN(lead_middle);
+ lead_middle_len = lead_end_len;
same here
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Copilot
@Copilot commented on this pull request.
Pull request overview
Refactors option-string parsing and path construction to consistently use the return value of copy_option_part() (and related length-tracking), reducing repeated STRLEN() calls and making several buffers length-aware via string_T.
Changes:
- Use
copy_option_part()return lengths instead of recomputing withSTRLEN()in multiple call sites. - Replace several ad-hoc
add_pathsep()/STRCAT()patterns withafter_pathsep()and direct copies at known offsets. - Introduce
string_Tin a few functions to track allocated buffers and their current lengths.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file| File | Description |
|---|---|
| src/viminfo.c | Uses copy_option_part() return length for prefix matching. |
| src/spellfile.c | Switches local buffer handling to string_T and uses safe length-appending. |
| src/spell.c | Uses returned lengths from copy_option_part() for spelllang/spellfile parsing and suffix appends. |
| src/scriptfile.c | Converts local path buffer to string_T, precomputes lengths, and replaces add_pathsep() usage. |
| src/optionstr.c | Uses string_T buffer and after_pathsep() for MYVIMDIR export logic. |
| src/memline.c | Uses string_T for directory part handling and replaces vim_strsave() with vim_strnsave() for literals. |
| src/help.c | Tracks NameBuff length from copy_option_part() to append separators/patterns without extra STRLEN(). |
| src/cindent.c | Uses copy_option_part() return length for comment leader comparisons. |
| src/change.c | Uses copy_option_part() return length for comment leader replacement length. |
| src/bufwrite.c | Uses returned lengths from copy_option_part() to compute end pointers without STRLEN(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
In src/memline.c:
> + names[1] = vim_strnsave((char_u *)".*.sw?", STRLEN_LITERAL(".*.sw?$"));
+ names[2] = vim_strnsave((char_u *)".sw?", STRLEN_LITERAL(".sw?"));
The STRLEN_LITERAL() argument doesn't match the actual string passed to vim_strnsave(): the saved string is "..sw?" but the length is computed from "..sw?$" (note the extra '$' and different length). Passing a larger length than the string literal can cause an out-of-bounds read in vim_strnsave(). Use a matching literal (or STRLEN_LITERAL(".*.sw?")) so the length is correct.
In src/memline.c:
> #else
- names[0] = vim_strsave((char_u *)"*.sw?");
+ names[0] = vim_strnsave((char_u *)"*.sw?", STRLEN_LITERAL("*_sw?"));
This vim_strnsave() call uses STRLEN_LITERAL("_sw?") for the length but the actual string being saved is ".sw?". While the lengths happen to match today, it's error-prone and can easily become an out-of-bounds read if either string changes. Use STRLEN_LITERAL("*.sw?") (or otherwise ensure the length corresponds to the same literal).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
John Marriott
@basilisk0315 commented on this pull request.
In src/memline.c:
> #endif
#if defined(UNIX) || defined(MSWIN)
// For Unix names starting with a dot are special. MS-Windows
// supports this too, on some file systems.
- names[1] = vim_strsave((char_u *)".*.sw?");
- names[2] = vim_strsave((char_u *)".sw?");
+ names[1] = vim_strnsave((char_u *)".*.sw?", STRLEN_LITERAL(".*.sw?$"));
Yep. Typo.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
John Marriott
@basilisk0315 commented on this pull request.
In src/bufwrite.c:
> @@ -1568,10 +1577,17 @@ buf_write(
while (*dirp)
{
// Isolate one directory name and make the backup file name.
- (void)copy_option_part(&dirp, IObuff, IOSIZE, ",");
+#if defined(UNIX) || defined(MSWIN)
+ int IObufflen;
+
+ IObufflen =
+#else
+ (void)
+#endif
Yeah to avoid warnings.
My compiler (clang) does not complain after adding UNUSED. I did a few tests and it seems to work ok.
According to sources I found (see: https://dev.to/martinlicht/unused-variables-in-cc-why-and-how-4cm4)
this should work, but I only have access to Windows and Linux so I am unable to check other platforms.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
John Marriott
@basilisk0315 commented on this pull request.
In src/cindent.c:
> if (what == COM_START)
{
STRCPY(lead_start, lead_end);
- lead_start_len = (int)STRLEN(lead_start);
+ lead_start_len = lead_end_len;
The STRCPY() statement (just above the assignment) copies lead_end into lead_start in it entirety. Which means it's length is the same too.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
John Marriott
> start_off = off;
start_align = align;
}
else if (what == COM_MIDDLE)
{
STRCPY(lead_middle, lead_end);
- lead_middle_len = (int)STRLEN(lead_middle);
+ lead_middle_len = lead_end_len;
Same here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
John Marriott
@basilisk0315 pushed 1 commit.
- 3f61713 Changes/fixes based on feedback
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
John Marriott
@basilisk0315 pushed 1 commit.
- 6b4da3f Fix based on feedback
You are receiving this because you are subscribed to this thread.