Viml: null list, add() and setline() #7695
Conversation
add(v:null_list, 1) does not fail. It should fail and return 1.
setline(1, v:_null_list) would crash.
| static void f_add(typval_T *argvars, typval_T *rettv, FunPtr fptr) | ||
| { | ||
| list_T *l; | ||
| const char *const arg_errmsg = N_("add() argument"); |
There was a problem hiding this comment.
You don’t need the variable unless it is used in multiple places.
There was a problem hiding this comment.
I see it actually could be used in multiple places. Though I think it is better to remove else clause, as well as the check for NULL and add tv_list_locked function near tv_list_len with similar contents and purpose: in any case I was planning to eventually remove any references outside of typval.[ch] to any list and dictionary internal structure members so it would be possible to play with their implementation.
There was a problem hiding this comment.
I have two questions regarding this comment:
First about removing the else clause: I'm not sure to which occurrence of tv_list_len you refer. My interpretation currently is that instead of the current if {if something/else error} else error, you would like the errors in f_add to be grouped right? So more like if () {error} else {perform actions}. Is this what you mean?
Second about 'misusing' tv_check_fixed to generate an error using VAR_FIXED. I could duplicate the define of 'E742' from the tv_check_fixed to the top of eval.c, where e_listreq is also defined (and perhaps sort those on error number), and then replace all those misuses, reducing the amount of calls to tv_ functions by 3. Do you like the idea?
There was a problem hiding this comment.
You added only one else to this function, it can be removed if you replace l->lv_lock with (to be created) tv_list_locked and remove the NULL check above. Do not duplicate the define, use tv_check_lock, but only once.
| rettv->vval.v_number = 1; /* Default: Failed */ | ||
| if (argvars[0].v_type == VAR_LIST) { | ||
| if ((l = argvars[0].vval.v_list) != NULL | ||
| && !tv_check_lock(l->lv_lock, "add() argument", TV_TRANSLATE)) { |
There was a problem hiding this comment.
(And that other place is missing N_() for some reason. Most likely an artifact from a massive non-automated refactoring performed by me.)
There was a problem hiding this comment.
Depending on the answers to the questions above, I'll add the N_, or change this line.
| li = l->lv_first; | ||
| if (l != NULL) { | ||
| li = l->lv_first; | ||
| } |
There was a problem hiding this comment.
This will prevent a crash, but echo setline(1, []) will still be different from echo setline(1, v:_null_list) (latter will yield error).
|
I am closing this as fixes both to add() and setline() are in #7708 and you can’t get a good fix without that PR’s first commits anyway. |
2 participants
See #4615 .
For the add function I tried to mimic the behaviour of the extend function.