[vim/vim] Vim9 class: public/protected member name clash needs its own error (PR #20277)
h_east
Vim9 class: public/protected member name clash needs its own error
Problem: When a public member and a protected member in a Vim9 class
have the same name (differing only in the leading '_'), Vim
reports E1369 "Duplicate variable", which is also used for
plain duplicate definitions. Users cannot tell from the
message whether the conflict is the public/protected naming
rule or a real duplicate.
Solution: Add a dedicated error E1406 "Public and protected member
have the same name" and emit it only when the name clash is
between a public and a protected member. Keep E1369 for
genuine duplicate variable definitions.
Fixes: #20240
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/20277
Commit Summary
- 6ec1811 Vim9 class: public/protected member name clash needs its own error
File Changes
(4 files)
- M runtime/doc/vim9class.txt (2)
- M src/errors.h (3)
- M src/testdir/test_vim9_class.vim (12)
- M src/vim9class.c (26)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
h_east
@h-east pushed 1 commit.
- fa5263b Vim9 class: public/protected member name clash needs its own error
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#20277)Hmm. It looks like v9.1.0600 removed several errors from error.h, but left them in the documentation:
E614, E1319, E1321, E1323, E1395, and E1406.
I noticed this because E1406 was reused in this change.
Also, while E1395 has already been reused, its tag label (*E1395*) seems to be misplaced.
I'll submit a separate PR to fix these issues shortly.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/vim9class.c:
> @@ -478,12 +479,17 @@ extends_check_dup_members(
// Compare against all the members in the parent class
for (int p_i = 0; p_i < p_member_count; p_i++)
{
- ocmember_T *p_m = p_members + p_i;
- char_u *qstr = (*p_m->ocm_name.string == '_')
- ? p_m->ocm_name.string + 1 : p_m->ocm_name.string;
+ ocmember_T *p_m = p_members + p_i;
+ bool p_protected = (*p_m->ocm_name.string == '_');
By convention, the prefix "p" is used for pointer variable names. In this case, it is used for a Boolean variable. Can some other prefix be used here?
In src/vim9class.c:
> int dup = FALSE; + bool p_protected = false;
In the extends_check_dup_members() function, the variables c_protected and p_protected are used. In this function, the variables p_protected and q_protected are used. For consistency reasons, can the same variable names be used in both the functions?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
In src/vim9class.c:
> @@ -478,12 +479,17 @@ extends_check_dup_members(
// Compare against all the members in the parent class
for (int p_i = 0; p_i < p_member_count; p_i++)
{
- ocmember_T *p_m = p_members + p_i;
- char_u *qstr = (*p_m->ocm_name.string == '_')
- ? p_m->ocm_name.string + 1 : p_m->ocm_name.string;
+ ocmember_T *p_m = p_members + p_i;
+ bool p_protected = (*p_m->ocm_name.string == '_');
Thanks for the review.
I don't think there is such a convention for prefixes. It would make more sense as a suffix. Alternatively, simply using p or ptr would be fine.
The lack of consistency — sometimes putting type information in the prefix and other times in the suffix — is likely what's hurting the maintainability of the code.
Also, following your logic, p_member_count on L470 and p_i on L480 (both int, in the same function) would also be considered "Bad."
Regarding the cross-function consistency: in extends_check_dup_members(), the outer loop uses c_ (child class members) and the inner loop uses p_ (parent class members), so c_protected / p_protected follow that pairing. In is_duplicate_variable(), the existing local convention is pstr (outer, the new variable being declared) and qstr (inner, the existing member being scanned), so p_protected / q_protected follow that — they should not be difficult to read in context.
A more substantive maintainability issue here is actually the mismatch within extends_check_dup_members() itself: the outer loop uses the c_ prefix on its member pointer (c_m), but its derived string is named pstr rather than cstr. So the existing code already mixes two naming schemes (c_/p_ and p/q) within the same function.
Either way, it seems inconsistent to flag only the newly introduced names when the same patterns already exist throughout these functions.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
h_east
> int dup = FALSE; + bool p_protected = false;
See above response.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#20277)I am fine with those style inconsistencies ;) We also need to re-generate the vim.pot file. I'll do it when merging. Thanks both!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
Thanks for providing the context and pointing out the existing inconsistencies. I agree this needs some improvement. I will look into this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.