[vim/vim] Use string_T to store ocm_name (PR #18973)

6 views
Skip to first unread message

John Marriott

unread,
Dec 19, 2025, 5:00:47 PM (20 hours ago) Dec 19
to vim/vim, Subscribed

Use struct string_T to store field ocn_name in struct ocmember_T.

This means we can use the .length field in struct string_T.

In addition in vim9class.c:
1: change some calls from ga_concat() to ga_concat_len() where the length is known.
2: in ex_class() remove unneeded variable name.
3. in intf_method_present() remove unneeded variable cl_name.
4: in update_member_method_lookup_table() move some assignments from inner loops into outer loops.
5: in member_lookup() remove unneeded else.
6: in is_duplicate_variable() and is_duplicate_enum() rearrange the string comparisons to first check the lengths match.

Cheers
John

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

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

Commit Summary

  • a010d95 Use string_T to store ocm_name

File Changes

(7 files)

Patch Links:

—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/18973@github.com>

Copilot

unread,
10:45 AM (3 hours ago) 10:45 AM
to vim/vim, Subscribed

@Copilot commented on this pull request.

Pull request overview

This PR refactors the ocmember_T struct to use string_T for the ocm_name field instead of a raw char_u* pointer. This allows the code to leverage the .length field for optimizations and cleaner comparisons.

Key Changes:

  • Changed ocm_name from char_u* to string_T struct (containing .string and .length fields)
  • Updated all access patterns to use .string when accessing the string pointer and .length when accessing the length
  • Optimized string operations by using ga_concat_len() instead of ga_concat() where lengths are known
  • Optimized duplicate checks in is_duplicate_variable() and is_duplicate_enum() by comparing lengths before string content
  • Removed unnecessary local variables (name in ex_class(), cl_name in intf_method_present())
  • Moved variable assignments out of inner loops in update_member_method_lookup_table()
  • Removed unnecessary else clause in member_lookup()

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/structs.h Changed ocm_name field type from char_u* to string_T
src/vim9class.c Updated all member name allocations, accesses, and comparisons to use string_T; added optimizations using length-based comparisons and ga_concat_len()
src/vim9expr.c Updated member name accesses to use .string field
src/vim9execute.c Updated member name accesses to use .string field
src/vim9compile.c Updated member name accesses to use .string field
src/userfunc.c Updated member name comparison to use .string field
src/eval.c Updated member name accesses to use .string field

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

In src/vim9class.c:

> +    if (m->ocm_name.string == NULL)
+	m->ocm_name.length = 0;

When vim_strnsave() fails and returns NULL, the function should return FAIL to indicate memory allocation failure. The function documentation states "Returns OK on success and FAIL on memory allocation failure". Currently, it sets length to 0 and continues, which leaves the member in an inconsistent state and incorrectly returns OK.

⬇️ Suggested change 
-    if (m->ocm_name.string == NULL)
-	m->ocm_name.length = 0;
+    if (m->ocm_name.string == NULL)
+    {
+	m->ocm_name.length = 0;
+	return FAIL;
+    }

In src/vim9class.c:

> +	m->ocm_name.string = vim_strnsave(m->ocm_name.string, m->ocm_name.length);
+	if (m->ocm_name.string == NULL)
+	    m->ocm_name.length = 0;

When vim_strnsave() fails and returns NULL while copying parent member names, this should be treated as a memory allocation failure. The function should return FAIL rather than setting length to 0 and continuing. Additionally, if allocation fails during member copying, previously allocated members should be cleaned up before returning FAIL.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/pull/18973/review/3601159539@github.com>

Christian Brabandt

unread,
10:58 AM (2 hours ago) 10:58 AM
to vim/vim, Subscribed
chrisbra left a comment (vim/vim#18973)

thanks

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

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

Christian Brabandt

unread,
11:21 AM (2 hours ago) 11:21 AM
to vim/vim, Subscribed

Closed #18973 via 87c6eb8.

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

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

Reply all
Reply to author
Forward
0 new messages