[PATCH 1 of 8] bdiff: add _version to help detect breaking binary changes

Jun Wu quark at fb.com
Tue May 2 20:56:31 EDT 2017


Excerpts from Gregory Szorc's message of 2017-05-02 17:41:21 -0700:
> I like the spirit of this series.
> 
> Have you given any consideration to:
> 
> * Sharing a version number across all modules? That will make the code
> easier at the expense of recompiling all C modules when not strictly
> necessary. Since the base rate of changing C modules is low and everything
> but zstd compiles almost instantaneously, one can make an argument that
> simplicity is sufficient.

While I think that also works, I think the benefit (less repetitive code) is
not that significant given the fact that I think CPython C code is already
cumbersome (look at those PyInits), so I won't optimize code length for them
as much as I do for non-Python C code.

I also slightly prefer the explicit way of saying which modules we are
checking and their expected versions in Patch 8.

> * A shared function or macro for calling PyModule_AddIntConstant(). It
> feels like a bit of redundant code across all the modules.

Suppose we have a .h file for that feature, then .c file still need roughly
same number of lines of change:

  +#include "...."
  +PyObject *m;
  +m = PyModule_Create(&bdiff_module);  // Python 2
  +define_version(m)                    // Python 2
  +define_version(m)                    // Python 3

Maybe we can unified the Py2 vs Py3 module definition. But that seems to be
another series which should not block this one.


More information about the Mercurial-devel mailing list