[RDY] viml: Add test for filter on v:_null_list #7477
Conversation
|
Don’t alter src/nvim/testdir with anything Neovim-specific, that is legacy Vim test suite. All the NULL checks are currently in test/functional/eval/null_spec.lua, commented with FIXME’s and given that tests are not failing problem is not fixed. |
|
Also you must not use |
|
[Edit]: I just noticed my repo history is incorrect even though I thought I had it fixed locally. I don't have time anymore to look into it now, but will try to fix it in the weekend. Thank you for the feedback. I had completely looked over/forgotten about the new
|
|
test/functional/eval/null_spec.lua
Outdated
| -- FIXME filter() should at least return L | ||
| null_expr_test('makes filter() return v:_null_list', 'map(L, "1") is# L', 0, 0) | ||
| null_expr_test('makes map() return v:_null_list', 'map(L, "v:val") is# L', 0, 1) | ||
| null_expr_test('makes filter() return v:_null_list', 'map(L, "1") is# L', 0, 1) |
There was a problem hiding this comment.
Here is a copy-paste typo, test needs to test filter(). And, BTW, list with OK tests is below, in this part only tests that need fixing are located. You may see “Incorrect behaviour” comment above.
src/nvim/testdir/test_filter_map.vim
Outdated
| call assert_equal([2, 3, 4], filter([1, 2, 3, 4], 'v:val > 1')) | ||
| call assert_equal([3, 4], filter([1, 2, 3, 4], 'v:key > 1')) | ||
| call assert_equal([], filter([1, 2, 3, 4], 0)) | ||
| call assert_equal(v:_null_list, filter(v:_null_list, 'v:val')) |
There was a problem hiding this comment.
This still needs not be here.
src/nvim/eval.c
Outdated
| if ((l = argvars[0].vval.v_list) == NULL | ||
| || (!map && tv_check_lock(l->lv_lock, arg_errmsg, TV_TRANSLATE))) { | ||
| l = argvars[0].vval.v_list; | ||
| if (l != NULL && !map && tv_check_lock(l->lv_lock, arg_errmsg, TV_TRANSLATE)) { |
There was a problem hiding this comment.
I would suggest to leave check as-is. Just move tv_copy() here and into the VAR_DICT case to leave only invalid inputs with zero return, modifications are in-place in any case. This also means that you need not modify anything else at all, only one line transition to two places.
src/nvim/eval.c
Outdated
| } else if (argvars[0].v_type == VAR_DICT) { | ||
| if ((d = argvars[0].vval.v_dict) == NULL | ||
| || (!map && tv_check_lock(d->dv_lock, arg_errmsg, TV_TRANSLATE))) { | ||
| tv_copy(&argvars[0], rettv); |
There was a problem hiding this comment.
Here you have three tv_copy() calls like if that needs to be called before return for some reason; I meant that in the surrounding if(), but with the bottommost one removed: like if saying that first argument is always returned if it has valid type.
|
Sorry for the delay, had to allocate time to different things in the past three weeks. I think this is what you meant, right? |
filter('v:_null_list, 'v:val') should return v:_null_list and a similar
statement should hold for map.
Changes after review
* Test inserted in legacy test suite has been removed by reverting the commit
adding it.
* Change the fix to tv_copy the argument before returning.
* Readd the two tests on crashes, and modified their expected return value.
* Move the test from 'incorrect behaviour' section to 'correct behaviour section'
* Add analogous tests for v:_null_dict
Always copy list or dictionary to return variable
If the type of input is correct (i.e. either a list or a dictionary), this
should also be returned.
|
Ok, I retrieved the latest changes in the repository, and then rebased and force pushed. As far as I understand it, this means it is ready to be merged. However, do note that this is my first time working non-locally with git. |
fvkluck
changed the title
4 participants
filter(v:_null_list, 'v:val') currently returns 0, but should return v:_null_list. Ref #4615