[PATCH 10 of 10 V2] util: remove compressorobj API from compression engines

Augie Fackler raf at durin42.com
Thu Nov 10 11:10:09 EST 2016


On Thu, Nov 10, 2016 at 10:55:17AM -0500, Augie Fackler wrote:
> On Wed, Nov 09, 2016 at 03:48:39PM -0800, Gregory Szorc wrote:
> > On Wed, Nov 9, 2016 at 9:35 AM, Augie Fackler <raf at durin42.com> wrote:
> >
> > > On Mon, Nov 07, 2016 at 07:13:58PM -0800, Gregory Szorc wrote:
> > > > # HG changeset patch
> > > > # User Gregory Szorc <gregory.szorc at gmail.com>
> > > > # Date 1478573874 28800
> > > > #      Mon Nov 07 18:57:54 2016 -0800
> > > > # Node ID 6755052bb245a2fbf30a22c8b4cf5599441fabfb
> > > > # Parent  8e2ca8bfa2d0f7c02b6f58b36341b4c4cf7ec0f9
> > > > util: remove compressorobj API from compression engines
> > >
> > > I like it. Let's chat some about the interface defined in patch 1
> > > (class scope invites questions, and none of those instances actually
> > > use the self attribute so I'd like to try and get things demoted to
> > > module level unless there's a compelling reason), and then get this
> > > landed. Thanks!
> > >
> >
> > It was originally less heavyweight and Pierre-Yves's review feedback
> > steered me towards the current implementation...
> >
> > I agree the classes are essentially singletons and don't provide much
> > value. But no matter how you slice it, you need a way to hold references to
> > a collection of functions for a particular engine. That's, uh, what a class
> > is for. I suppose we could pass function "pointers" to a class's __init__
> > and have it populate attributes. But the base class with stub methods does
> > provide some safety and an obvious place to document the API. And the final
> > result would probably have the same external API, just N instances of a 1
> > class instead of 1 instances of N classes.
> >
> > FWIW, I anticipate the zstd engine will need "self" storage. This is to
> > facilitate C vs CFFI and lazy module importing and possibly caching of
> > certain objects to facilitate faster revlog operations.
>
> If the zstd thing will need self-storage in an instance, then I'm okay
> to take this as-is and clean it up to be less classy if we end up not
> needing it.

(Which is to say, queued)


More information about the Mercurial-devel mailing list