[vim/vim] fix: improve memory management and code style (PR #17984)
glepnir
Problem:
- Missing cleanup of lmatchpos lists causing memory leaks
- Missing error handling for list operations
- Use of malloc() instead of Vim's alloc() functions
- Inconsistent C-style comments
- Missing null pointer checks for memory allocation
- Incorrect use of vim_free() for list objects
Solution:
- Add proper cleanup of lmatchpos in done section using list_unref()
- Set lmatchpos to NULL after successful transfer to avoid confusion
- Add error handling for list_append_tv() failures
- Replace malloc() with alloc() and add null pointer checks
- Convert C-style comments to C++ style for consistency
- Fix vim_free() calls to use list_free() for list objects
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/17984
Commit Summary
- 20d4dae fix: improve memory management and code style
File Changes
(1 file)
- M src/fuzzy.c (149)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
@zeertzjq commented on this pull request.
In src/fuzzy.c:
> + + if (items[i].lmatchpos) + list_unref(items[i].lmatchpos);
I don't think using list_unref() here makes sense. If list_append_list() fails, the remaining lists' reference counts are 0, so it's better to just use list_free() directly.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
glepnir
@glepnir commented on this pull request.
when 1 fails, 0 succeeds. the li->lv_refcount of 0 is 1. so using list_unref here is more safer 🤔
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
zeertzjq
@zeertzjq commented on this pull request.
I can't understand what you are talking about.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
glepnir
@glepnir pushed 1 commit.
- 465a7e9 fix: improve memory management and code style
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
zeertzjq
@zeertzjq commented on this pull request.
In src/fuzzy.c:
> + + if (items[i].lmatchpos) + list_unref(items[i].lmatchpos);
No, if list_append_list() succeeds, you already set items[i].lmatchpos to NULL, so this isn't called. Furthermore, for lists that are successfully added to retlist, they should be freed along with retlist (which is by the caller), not here.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
@zeertzjq commented on this pull request.
In src/fuzzy.c:
> + + if (items[i].lmatchpos) + list_unref(items[i].lmatchpos);
And you also need to change list_unref to list_free in the commit message.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
glepnir
@glepnir commented on this pull request.
yes, they should be free by fmatchlist. I have modified and updated
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#17984)thanks
—
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.