On 30-Apr-15, Nikolay Pavlov wrote:
> 2015-04-30 10:56 GMT+03:00 Olaf Dabrunz <o...@fctrace.org>:
> > diff -r 18d84ed365a5 src/testdir/test55.in
> > --- a/src/testdir/test55.in Wed Apr 22 22:18:22 2015 +0200
> > +++ b/src/testdir/test55.in Thu Apr 30 09:09:37 2015 +0200
> > @@ -408,6 +408,20 @@ let l = [0, 1, 2, 3]
> > :endtry
> > :$put =string(d)
> > :"
> > +:$put ='extend() after lock on dict (existing: yes, new: no):'
> > +:unlet! d
> > +:let d = {'a': 99, 'b': 100}
> > +:lockvar 1 d
> > +:try
> > +: $put =string(extend(d, {'a': 123}))
> > +: $put ='did extend() existing item'
>
> I think :try should be here, not above.
The :try block fails on the first extend() without the patch, and on the
second one (as intended) with the patch. This tests for regressions.
What would be achieved by moving the :try here?
> > +: $put =string(extend(d, {'x': 'new'}))
> > +: $put ='did extend() with new item'
> > +:catch
> > +: $put =v:exception[:14]
> > +:endtry
> > +:$put =string(d)
> > +:"
>
> Here some crucial tests are missing:
>
> 1. `extend(d, {'a': 456, 'x': 'new'})` without try/catch.
> 2. Same in a try/catch.
> 3. Both repeated with a second argument to `extend` where `keys(arg)`
> returns `[old_key1, new_key2, old_key3]` (this is needed to make sure
> that all old keys are modified).
>
> Unless it is possible for `extend` to modify *all* old keys regardless
> the internal order I would prefer the current behaviour where you at
> least always know that nothing will be modified.
I sent a new patch (in a different mail) that changes all of the
pre-existing and added error-on-exit behavior into all-or-nothing
behavior, except for the out of memory case (which remains unchanged at
continue-on-error). Please see my comments and the patch there.
The patch includes a new test for all-or-nothing semantics, as you
described in 3. above.
Could you explain what tests 1. and 2. are meant to test?
> It is a completely
> useless behaviour to partially modify a dictionary depending on key
> ordering which is not controlled by a plugin developer.
But so far, this is the behavior implemented in extend() for all error
conditions that can occur, except for out of memory (which uses
continue-on-error).
Please see my comments in the reply to Bram's mail.
My position on this is this:
- The error conditions in extend() and other list/dict functions
do mostly occur under exceptional circumstances, or when the user
is asking for them and is prepared to handle them:
Programming errors (exceptional):
- adding a function to scopes l: or g: that conflicts with a
builtin function
- adding or replacing an item in a variable scope, when the key
is not a valid variable name
- add/del/change on fixed/readonly variable scope or variable
This rarely happens (see other thread about checking for OOM):
- out of memory
Programmer is asking for error reports:
- using "error" mode instead of "force" or "keep"
- add/del/change on user-locked variable during debugging
Non-exceptional:
- add/del/change on user-locked variable in production code
- All-or-nothing behavior is expensive.
Dictionary accesses must be done twice, or a cache must be
created, and to handle OOM we must even implement rollback (which
needs copies of changed items).
For the second version of my extend() lock patch that uses a
separate checking loop, according to my measurements (see the
other mail) the time spent in extend() doubles.
I have not assessed the costs for other list/dict functions.
- User locks can be used for debugging, and then the programmer
should handle the error reports.
I see potential for using user locks in debugging autocommand
interactions.
- User locks can be used in production code.
The current exit-on-error behavior requires :try blocks and error
handling. All-or-nothing behavior can be implemented by working
on a copy() of the dictionary.
This is not convenient or efficient.
Because of the costs of all-or-nothing behavior (at least on
dictionaries), I would prefer if this is not the default behavior.
But for using user locks in production code, it would be good if we
could choose continue-on-error mode or all-or-nothing mode.
Maybe as new values for the third parameter to extend(): "continue"
and "rollback" or something like that.
For other list/dict functions, maybe something similar could be
done.
> Test should also test the order of the `keys()` output to make sure
> that it will be modified once changes that affect internal hash
> ordering are pushed.
For the change on extend() locks, what would testing for changes in hash
ordering achieve?
We know that extend()'s current exit-on-error behavior depends on hash
ordering. Of course, when the hash ordering changes, the item that
causes an exit-on-error may come sooner or later. But this is not a
regression.
--
Olaf Dabrunz (oda <at> fctrace.org)
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to vim_dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
