2015-05-02 5:44 GMT+03:00 Olaf Dabrunz <o...@fctrace.org>:
> 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?
With `:try` block moved it will run both extend()s always. Test will
fail in any case: without move because of absense of two put lines and
also different dictionary value, with move because of wrong put output
when stringifying dictionary later and put=string(extend(...)) output
(with the error, but without :try it will put zero). By moving try you
show *where you expect* the exception.
>
>> > +: $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?
If you were going to use not all-or-nothing, but continue-on-error
they were testing how you continue on error specifically (that you do
this regardless of :try).
>
>> 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?
That if you have continue-on-error you *do* have continue-on-error.
Tests for changes in hash ordering are needed to check that test is
testing what it was expected to test: [existing, new, existing] in a
second dictionary makes sure that both existing keys are modified even
if error occures in between. If you do not check the ordering you
cannot know whether you are testing [existing, new, existing] or
[existing, existing, new] (which is useless because it will emit the
same result for both continue-on-error and exit-on-error).
>
> 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.
It is a regression *of the test itself*, not the regression in the code.
If you prefer exit-on-error like it is currently doing [existing, new,
existing] will fail if behaviour will change compared to both
continue-on-error and all-or-nothing. [new, existing, existing] does
not allow to distinguish all-or-nothing and exit-on-error. [existing,
existing, new] does not allow to distinguish exit-on-error and
continue-on-error. So you need to check the ordering to make sure your
test is testing what it is expected to test.
In the test in the next message you should check that 'c' is both not
the first and not the last key, not just not the first.
>
> --
> 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.
--
--
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.
