Patchwork [05,of,10,PyPy] histedit: don't bother with cPickle, demand-load pickle

login
register
mail settings
Submitter Bryan O'Sullivan
Date Dec. 24, 2015, 12:22 a.m.
Message ID <921ca7a0d4bb771505fd.1450916540@bryano-mbp.local>
Download mbox | patch
Permalink /patch/12339/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Bryan O'Sullivan - Dec. 24, 2015, 12:22 a.m.
We're unlikely to ever need the pickle module, so there's no good
reason to force loading of its faster cousin.
Yuya Nishihara - Dec. 27, 2015, 4:20 p.m.
On Wed, 23 Dec 2015 16:22:20 -0800, Bryan O'Sullivan wrote:
> We're unlikely to ever need the pickle module, so there's no good
> reason to force loading of its faster cousin.
> 
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -169,11 +169,7 @@ the drop to be implicit for missing comm
>  
>  """
>  
> -try:
> -    import cPickle as pickle
> -    pickle.dump # import now
> -except ImportError:
> -    import pickle
> +import pickle

We generally use cPickle without fallback. I'll change it in flight to use
cPickle.
Augie Fackler - Dec. 27, 2015, 5:35 p.m.
On Mon, Dec 28, 2015 at 01:20:39AM +0900, Yuya Nishihara wrote:
> On Wed, 23 Dec 2015 16:22:20 -0800, Bryan O'Sullivan wrote:
> > We're unlikely to ever need the pickle module, so there's no good
> > reason to force loading of its faster cousin.
> >
> > diff --git a/hgext/histedit.py b/hgext/histedit.py
> > --- a/hgext/histedit.py
> > +++ b/hgext/histedit.py
> > @@ -169,11 +169,7 @@ the drop to be implicit for missing comm
> >
> >  """
> >
> > -try:
> > -    import cPickle as pickle
> > -    pickle.dump # import now
> > -except ImportError:
> > -    import pickle
> > +import pickle
>
> We generally use cPickle without fallback. I'll change it in flight to use
> cPickle.
>

I believe cPickle is problematic on PyPy somehow (buggy or not there),
so that's the wrong fix in this case.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Yuya Nishihara - Dec. 28, 2015, 1:01 p.m.
On Sun, 27 Dec 2015 12:35:41 -0500, Augie Fackler wrote:
> On Mon, Dec 28, 2015 at 01:20:39AM +0900, Yuya Nishihara wrote:
> > On Wed, 23 Dec 2015 16:22:20 -0800, Bryan O'Sullivan wrote:
> > > We're unlikely to ever need the pickle module, so there's no good
> > > reason to force loading of its faster cousin.
> > >
> > > diff --git a/hgext/histedit.py b/hgext/histedit.py
> > > --- a/hgext/histedit.py
> > > +++ b/hgext/histedit.py
> > > @@ -169,11 +169,7 @@ the drop to be implicit for missing comm
> > >
> > >  """
> > >
> > > -try:
> > > -    import cPickle as pickle
> > > -    pickle.dump # import now
> > > -except ImportError:
> > > -    import pickle
> > > +import pickle
> >
> > We generally use cPickle without fallback. I'll change it in flight to use
> > cPickle.
> >
> 
> I believe cPickle is problematic on PyPy somehow (buggy or not there),
> so that's the wrong fix in this case.

Hmm, it seems both cPicke and pickle don't handle invalid input well on PyPy.
But I agree there's no reason to stick to cPickle anyway, so queued this as is.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -169,11 +169,7 @@  the drop to be implicit for missing comm
 
 """
 
-try:
-    import cPickle as pickle
-    pickle.dump # import now
-except ImportError:
-    import pickle
+import pickle
 import errno
 import os
 import sys