Patchwork sortdict: fix .pop() to return a value

login
register
mail settings
Submitter Yuya Nishihara
Date April 9, 2017, 1:10 p.m.
Message ID <48a7a1f77a9489e3c4b5.1491743401@mimosa>
Download mbox | patch
Permalink /patch/20036/
State Accepted
Headers show

Comments

Yuya Nishihara - April 9, 2017, 1:10 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1491706629 -32400
#      Sun Apr 09 11:57:09 2017 +0900
# Node ID 48a7a1f77a9489e3c4b5f862243782ceae80eaf9
# Parent  9259cf823690e4fcd34a4d2ecd57ced2060d2b3d
sortdict: fix .pop() to return a value

My future patch will need it.
David Soria Parra - April 10, 2017, 2:50 a.m.
On Sun, Apr 09, 2017 at 10:10:01PM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1491706629 -32400
> #      Sun Apr 09 11:57:09 2017 +0900
> # Node ID 48a7a1f77a9489e3c4b5f862243782ceae80eaf9
> # Parent  9259cf823690e4fcd34a4d2ecd57ced2060d2b3d
> sortdict: fix .pop() to return a value
> 
> My future patch will need it.

Generally this looks fine to me.

> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -555,11 +555,11 @@ class sortdict(dict):
>          dict.__delitem__(self, key)
>          self._list.remove(key)
>      def pop(self, key, *args, **kwargs):
> -        dict.pop(self, key, *args, **kwargs)
>          try:
>              self._list.remove(key)
>          except ValueError:
>              pass
> +        return dict.pop(self, key, *args, **kwargs)

Note that this breaks the insert/delete asymmetry. We usually insert into the
_list first and then add it to the dictionary and the current version of pop
deletes from the dictionary first and then from the list ensuring an

add to list
add to dict
del from dict
del from list

order. Your patch breaks this. I wonder if can lead to unwanted behavior. If we
want to be on the save side we should probably do

   res = dict.pop(...)
   try:
     self._list.remove()
    ...
   return res
Ryan McElroy - April 10, 2017, 9:22 a.m.
On 4/10/17 3:50 AM, David Soria Parra wrote:
> On Sun, Apr 09, 2017 at 10:10:01PM +0900, Yuya Nishihara wrote:
>> # HG changeset patch
>> # User Yuya Nishihara <yuya@tcha.org>
>> # Date 1491706629 -32400
>> #      Sun Apr 09 11:57:09 2017 +0900
>> # Node ID 48a7a1f77a9489e3c4b5f862243782ceae80eaf9
>> # Parent  9259cf823690e4fcd34a4d2ecd57ced2060d2b3d
>> sortdict: fix .pop() to return a value
>>
>> My future patch will need it.
> Generally this looks fine to me.
>
>> diff --git a/mercurial/util.py b/mercurial/util.py
>> --- a/mercurial/util.py
>> +++ b/mercurial/util.py
>> @@ -555,11 +555,11 @@ class sortdict(dict):
>>           dict.__delitem__(self, key)
>>           self._list.remove(key)
>>       def pop(self, key, *args, **kwargs):
>> -        dict.pop(self, key, *args, **kwargs)
>>           try:
>>               self._list.remove(key)
>>           except ValueError:
>>               pass
>> +        return dict.pop(self, key, *args, **kwargs)
> Note that this breaks the insert/delete asymmetry. We usually insert into the
> _list first and then add it to the dictionary and the current version of pop
> deletes from the dictionary first and then from the list ensuring an
>
> add to list
> add to dict
> del from dict
> del from list
>
> order. Your patch breaks this. I wonder if can lead to unwanted behavior. If we
> want to be on the save side we should probably do
>
>     res = dict.pop(...)
>     try:
>       self._list.remove()
>      ...
>     return res
>

Hm, given the single-threaded nature of the code here, I can't imagine 
how this would lead to a problem. Maybe my imagination isn't wild 
enough, though? I'll mark as "prereviewed" in patchwork but I'd also be 
okay with a v2 that implements David's suggestion.
Yuya Nishihara - April 10, 2017, 12:21 p.m.
On Sun, 9 Apr 2017 19:50:26 -0700, David Soria Parra wrote:
> On Sun, Apr 09, 2017 at 10:10:01PM +0900, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1491706629 -32400
> > #      Sun Apr 09 11:57:09 2017 +0900
> > # Node ID 48a7a1f77a9489e3c4b5f862243782ceae80eaf9
> > # Parent  9259cf823690e4fcd34a4d2ecd57ced2060d2b3d
> > sortdict: fix .pop() to return a value
> > 
> > My future patch will need it.
> 
> Generally this looks fine to me.
> 
> > diff --git a/mercurial/util.py b/mercurial/util.py
> > --- a/mercurial/util.py
> > +++ b/mercurial/util.py
> > @@ -555,11 +555,11 @@ class sortdict(dict):
> >          dict.__delitem__(self, key)
> >          self._list.remove(key)
> >      def pop(self, key, *args, **kwargs):
> > -        dict.pop(self, key, *args, **kwargs)
> >          try:
> >              self._list.remove(key)
> >          except ValueError:
> >              pass
> > +        return dict.pop(self, key, *args, **kwargs)
> 
> Note that this breaks the insert/delete asymmetry. We usually insert into the
> _list first and then add it to the dictionary and the current version of pop
> deletes from the dictionary first and then from the list ensuring an
> 
> add to list
> add to dict
> del from dict
> del from list
> 
> order. Your patch breaks this.

Yes, but anyway sortdict doesn't guarantee strong exception safety nor thread
safety, so the order shouldn't matter. The only practical reason why
__delitem__() calls dict first is to raise KeyError.
Augie Fackler - April 11, 2017, 3:04 p.m.
On Sun, Apr 09, 2017 at 10:10:01PM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1491706629 -32400
> #      Sun Apr 09 11:57:09 2017 +0900
> # Node ID 48a7a1f77a9489e3c4b5f862243782ceae80eaf9
> # Parent  9259cf823690e4fcd34a4d2ecd57ced2060d2b3d
> sortdict: fix .pop() to return a value

Queued, thanks

>
> My future patch will need it.
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -555,11 +555,11 @@ class sortdict(dict):
>          dict.__delitem__(self, key)
>          self._list.remove(key)
>      def pop(self, key, *args, **kwargs):
> -        dict.pop(self, key, *args, **kwargs)
>          try:
>              self._list.remove(key)
>          except ValueError:
>              pass
> +        return dict.pop(self, key, *args, **kwargs)
>      def keys(self):
>          return self._list[:]
>      def iterkeys(self):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -555,11 +555,11 @@  class sortdict(dict):
         dict.__delitem__(self, key)
         self._list.remove(key)
     def pop(self, key, *args, **kwargs):
-        dict.pop(self, key, *args, **kwargs)
         try:
             self._list.remove(key)
         except ValueError:
             pass
+        return dict.pop(self, key, *args, **kwargs)
     def keys(self):
         return self._list[:]
     def iterkeys(self):