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.