[PATCH 1 of 7 V2] util: add a macro initializing CPython modules

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1 of 7 V2] util: add a macro initializing CPython modules

Jun Wu
# HG changeset patch
# User Jun Wu <[hidden email]>
# Date 1494288921 25200
#      Mon May 08 17:15:21 2017 -0700
# Node ID 55ae1324c950e76270ccb0f68c098513bc36fc91
# Parent  78496ac300255e9996b3e282086661afc08af37c
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 55ae1324c950
util: add a macro initializing CPython modules

This will greatly simplify C module initialization code. The signature
requires a "version" field to help detect incompatible ".so" modules usually
caused by forgetting to build.

diff --git a/mercurial/util.h b/mercurial/util.h
--- a/mercurial/util.h
+++ b/mercurial/util.h
@@ -43,3 +43,47 @@ typedef unsigned char bool;
 #endif
 
+/* unify Python2 and Python3 module initialization.
+ * precheck and postinit are expressions, -1 indicates error. postinit could
+ * use "m" as the module object. precheck and postinit could be SKIP to
+ * indicate there is nothing to check or initialize.  */
+#define SKIP 0
+#ifdef IS_PY3K
+#define PYMODULEINIT(name, methods, doc, version, precheck, postinit) \
+ static struct PyModuleDef  name ## _module = { \
+ PyModuleDef_HEAD_INIT, \
+ #name, \
+ doc, \
+ -1, \
+ methods \
+ }; \
+ PyMODINIT_FUNC PyInit_ ## name(void) \
+ { \
+ PyObject *m; \
+ if ((precheck) == -1) \
+ return NULL; \
+ m = PyModule_Create(&(name ## _module)); \
+ if (m == NULL) \
+ return NULL; \
+ if (PyModule_AddIntConstant(m, "version", version) == -1) \
+ return NULL; \
+ if ((postinit) == -1) \
+ return NULL; \
+ return m; \
+ }
+#else
+#define PYMODULEINIT(name, methods, doc, version, precheck, postinit) \
+ PyMODINIT_FUNC (init ## name)(void) \
+ { \
+ PyObject *m; \
+ if ((precheck) == -1) \
+ return; \
+ m = Py_InitModule3(#name, methods, doc); \
+ if (m == NULL) \
+ return; \
+ if (PyModule_AddIntConstant(m, "version", version) == -1) \
+ return; \
+ (void)postinit; \
+ }
+#endif
+
 #endif /* _HG_UTIL_H_ */
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 2 of 7 V2] base85: use PYMODULEINIT

Jun Wu
# HG changeset patch
# User Jun Wu <[hidden email]>
# Date 1494289632 25200
#      Mon May 08 17:27:12 2017 -0700
# Node ID 407cb540b3a59ec291f2166d1592af78d992fbfb
# Parent  55ae1324c950e76270ccb0f68c098513bc36fc91
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 407cb540b3a5
base85: use PYMODULEINIT

diff --git a/mercurial/base85.c b/mercurial/base85.c
--- a/mercurial/base85.c
+++ b/mercurial/base85.c
@@ -19,5 +19,5 @@ static const char b85chars[] = "01234567
 static char b85dec[256];
 
-static void b85prep(void)
+static int b85prep(void)
 {
  unsigned i;
@@ -26,4 +26,6 @@ static void b85prep(void)
  for (i = 0; i < sizeof(b85chars); i++)
  b85dec[(int)(b85chars[i])] = i + 1;
+
+ return 0;
 }
 
@@ -158,25 +160,3 @@ static PyMethodDef methods[] = {
 };
 
-#ifdef IS_PY3K
-static struct PyModuleDef base85_module = {
- PyModuleDef_HEAD_INIT,
- "base85",
- base85_doc,
- -1,
- methods
-};
-
-PyMODINIT_FUNC PyInit_base85(void)
-{
- b85prep();
-
- return PyModule_Create(&base85_module);
-}
-#else
-PyMODINIT_FUNC initbase85(void)
-{
- Py_InitModule3("base85", methods, base85_doc);
-
- b85prep();
-}
-#endif
+PYMODULEINIT(base85, methods, base85_doc, 1, b85prep(), SKIP);
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 3 of 7 V2] bdiff: use PYMODULEINIT

Jun Wu
In reply to this post by Jun Wu
# HG changeset patch
# User Jun Wu <[hidden email]>
# Date 1494290228 25200
#      Mon May 08 17:37:08 2017 -0700
# Node ID 2387f3cfbf1ef2b8cfec514cc558d94dc257a219
# Parent  407cb540b3a59ec291f2166d1592af78d992fbfb
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 2387f3cfbf1e
bdiff: use PYMODULEINIT

diff --git a/mercurial/bdiff_module.c b/mercurial/bdiff_module.c
--- a/mercurial/bdiff_module.c
+++ b/mercurial/bdiff_module.c
@@ -193,21 +193,3 @@ static PyMethodDef methods[] = {
 };
 
-#ifdef IS_PY3K
-static struct PyModuleDef bdiff_module = {
- PyModuleDef_HEAD_INIT,
- "bdiff",
- mdiff_doc,
- -1,
- methods
-};
-
-PyMODINIT_FUNC PyInit_bdiff(void)
-{
- return PyModule_Create(&bdiff_module);
-}
-#else
-PyMODINIT_FUNC initbdiff(void)
-{
- Py_InitModule3("bdiff", methods, mdiff_doc);
-}
-#endif
+PYMODULEINIT(bdiff, methods, mdiff_doc, 1, SKIP, SKIP);
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 4 of 7 V2] diffhelpers: use PYMODULEINIT

Jun Wu
In reply to this post by Jun Wu
# HG changeset patch
# User Jun Wu <[hidden email]>
# Date 1494290124 25200
#      Mon May 08 17:35:24 2017 -0700
# Node ID bdb69cf7f0ff217074288b5f21bdfb28860358a0
# Parent  2387f3cfbf1ef2b8cfec514cc558d94dc257a219
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r bdb69cf7f0ff
diffhelpers: use PYMODULEINIT

The exception code was cleaned up a bit so it is also attached to the module
in Python 2.

diff --git a/mercurial/diffhelpers.c b/mercurial/diffhelpers.c
--- a/mercurial/diffhelpers.c
+++ b/mercurial/diffhelpers.c
@@ -165,35 +165,13 @@ static PyMethodDef methods[] = {
 };
 
-#ifdef IS_PY3K
-static struct PyModuleDef diffhelpers_module = {
- PyModuleDef_HEAD_INIT,
- "diffhelpers",
- diffhelpers_doc,
- -1,
- methods
-};
-
-PyMODINIT_FUNC PyInit_diffhelpers(void)
+static int postinit(PyObject *mod)
 {
- PyObject *m;
-
- m = PyModule_Create(&diffhelpers_module);
- if (m == NULL)
- return NULL;
+ diffhelpers_Error = PyErr_NewException("diffhelpers.diffhelpersError",
+       NULL, NULL);
+ if (!diffhelpers_Error)
+ return -1;
+ Py_INCREF(diffhelpers_Error);
+ return PyModule_AddObject(mod, "diffhelpersError", diffhelpers_Error);
+}
 
- diffhelpers_Error = PyErr_NewException("diffhelpers.diffhelpersError",
- NULL, NULL);
- Py_INCREF(diffhelpers_Error);
- PyModule_AddObject(m, "diffhelpersError", diffhelpers_Error);
-
- return m;
-}
-#else
-PyMODINIT_FUNC
-initdiffhelpers(void)
-{
- Py_InitModule3("diffhelpers", methods, diffhelpers_doc);
- diffhelpers_Error = PyErr_NewException("diffhelpers.diffhelpersError",
-                                        NULL, NULL);
-}
-#endif
+PYMODULEINIT(diffhelpers, methods, diffhelpers_doc, 1, SKIP, postinit(m));
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 5 of 7 V2] mpatch: use PYMODULEINIT

Jun Wu
In reply to this post by Jun Wu
# HG changeset patch
# User Jun Wu <[hidden email]>
# Date 1494290506 25200
#      Mon May 08 17:41:46 2017 -0700
# Node ID 5575f6c5d732a74001bcd96273b12d4d3f555649
# Parent  bdb69cf7f0ff217074288b5f21bdfb28860358a0
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 5575f6c5d732
mpatch: use PYMODULEINIT

A side effect of the clean up is mercurial.mpatch.mpatchError gets exposed,
which seems to be a good thing.

diff --git a/mercurial/mpatch_module.c b/mercurial/mpatch_module.c
--- a/mercurial/mpatch_module.c
+++ b/mercurial/mpatch_module.c
@@ -161,35 +161,13 @@ static PyMethodDef methods[] = {
 };
 
-#ifdef IS_PY3K
-static struct PyModuleDef mpatch_module = {
- PyModuleDef_HEAD_INIT,
- "mpatch",
- mpatch_doc,
- -1,
- methods
-};
-
-PyMODINIT_FUNC PyInit_mpatch(void)
+static int postinit(PyObject *mod)
 {
- PyObject *m;
-
- m = PyModule_Create(&mpatch_module);
- if (m == NULL)
- return NULL;
-
  mpatch_Error = PyErr_NewException("mercurial.mpatch.mpatchError",
   NULL, NULL);
+ if (!mpatch_Error)
+ return -1;
  Py_INCREF(mpatch_Error);
- PyModule_AddObject(m, "mpatchError", mpatch_Error);
-
- return m;
+ return PyModule_AddObject(mod, "mpatchError", mpatch_Error);
 }
-#else
-PyMODINIT_FUNC
-initmpatch(void)
-{
- Py_InitModule3("mpatch", methods, mpatch_doc);
- mpatch_Error = PyErr_NewException("mercurial.mpatch.mpatchError",
-  NULL, NULL);
-}
-#endif
+
+PYMODULEINIT(mpatch, methods, mpatch_doc, 1, SKIP, postinit(m));
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 6 of 7 V2] osutil: use PYMODULEINIT

Jun Wu
In reply to this post by Jun Wu
# HG changeset patch
# User Jun Wu <[hidden email]>
# Date 1494288978 25200
#      Mon May 08 17:16:18 2017 -0700
# Node ID 86b0d4567e2f7876a492ab2e60220cadb8191625
# Parent  5575f6c5d732a74001bcd96273b12d4d3f555649
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 86b0d4567e2f
osutil: use PYMODULEINIT

diff --git a/mercurial/osutil.c b/mercurial/osutil.c
--- a/mercurial/osutil.c
+++ b/mercurial/osutil.c
@@ -1302,27 +1302,7 @@ static PyMethodDef methods[] = {
 };
 
-#ifdef IS_PY3K
-static struct PyModuleDef osutil_module = {
- PyModuleDef_HEAD_INIT,
- "osutil",
- osutil_doc,
- -1,
- methods
-};
+static int precheck(void) {
+ return PyType_Ready(&listdir_stat_type);
+}
 
-PyMODINIT_FUNC PyInit_osutil(void)
-{
- if (PyType_Ready(&listdir_stat_type) < 0)
- return NULL;
-
- return PyModule_Create(&osutil_module);
-}
-#else
-PyMODINIT_FUNC initosutil(void)
-{
- if (PyType_Ready(&listdir_stat_type) == -1)
- return;
-
- Py_InitModule3("osutil", methods, osutil_doc);
-}
-#endif
+PYMODULEINIT(osutil, methods, osutil_doc, 1, precheck(), SKIP);
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 7 of 7 V2] parsers: use PYMODULEINIT

Jun Wu
In reply to this post by Jun Wu
# HG changeset patch
# User Jun Wu <[hidden email]>
# Date 1494290762 25200
#      Mon May 08 17:46:02 2017 -0700
# Node ID d449ad60b6ae74ddf7a6765ab2687a91435c62e9
# Parent  86b0d4567e2f7876a492ab2e60220cadb8191625
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r d449ad60b6ae
parsers: use PYMODULEINIT

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -2855,5 +2855,5 @@ void dirs_module_init(PyObject *mod);
 void manifest_module_init(PyObject *mod);
 
-static void module_init(PyObject *mod)
+static int module_init(PyObject *mod)
 {
  /* This module constant has two purposes.  First, it lets us unit test
@@ -2873,5 +2873,5 @@ static void module_init(PyObject *mod)
  if (PyType_Ready(&indexType) < 0 ||
     PyType_Ready(&dirstateTupleType) < 0)
- return;
+ return -1;
  Py_INCREF(&indexType);
  PyModule_AddObject(mod, "index", (PyObject *)&indexType);
@@ -2884,4 +2884,5 @@ static void module_init(PyObject *mod)
  if (nullentry)
  PyObject_GC_UnTrack(nullentry);
+ return 0;
 }
 
@@ -2912,32 +2913,4 @@ static int check_python_version(void)
 }
 
-#ifdef IS_PY3K
-static struct PyModuleDef parsers_module = {
- PyModuleDef_HEAD_INIT,
- "parsers",
- parsers_doc,
- -1,
- methods
-};
-
-PyMODINIT_FUNC PyInit_parsers(void)
-{
- PyObject *mod;
-
- if (check_python_version() == -1)
- return NULL;
- mod = PyModule_Create(&parsers_module);
- module_init(mod);
- return mod;
-}
-#else
-PyMODINIT_FUNC initparsers(void)
-{
- PyObject *mod;
-
- if (check_python_version() == -1)
- return;
- mod = Py_InitModule3("parsers", methods, parsers_doc);
- module_init(mod);
-}
-#endif
+PYMODULEINIT(parsers, methods, parsers_doc, 1, check_python_version(),
+     module_init(m));
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1 of 7 V2] util: add a macro initializing CPython modules

Yuya Nishihara
In reply to this post by Jun Wu
On Sat, 13 May 2017 11:55:23 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <[hidden email]>
> # Date 1494288921 25200
> #      Mon May 08 17:15:21 2017 -0700
> # Node ID 55ae1324c950e76270ccb0f68c098513bc36fc91
> # Parent  78496ac300255e9996b3e282086661afc08af37c
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 55ae1324c950
> util: add a macro initializing CPython modules

> +#define PYMODULEINIT(name, methods, doc, version, precheck, postinit) \
> + PyMODINIT_FUNC (init ## name)(void) \
> + { \
> + PyObject *m; \
> + if ((precheck) == -1) \
> + return; \
> + m = Py_InitModule3(#name, methods, doc); \
> + if (m == NULL) \
> + return; \
> + if (PyModule_AddIntConstant(m, "version", version) == -1) \
> + return; \
> + (void)postinit; \
> + }
> +#endif

Maybe I'm a bit conservative about a macro use, but my concern here is, it's
so easy to make postinint never be called.

  PYMODULEINIT(parsers, methods, parsers_doc, 1, check_python_version(),
               module_init)
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1 of 7 V2] util: add a macro initializing CPython modules

Jun Wu
Excerpts from Yuya Nishihara's message of 2017-05-14 12:57:57 +0900:
> > +        (void)postinit; \

This line could be changed to:
 
               if ((postinit) == -1) \
                       return; \

> > +    }
> > +#endif
>
> Maybe I'm a bit conservative about a macro use, but my concern here is,
> it's so easy to make postinint never be called.
>
>   PYMODULEINIT(parsers, methods, parsers_doc, 1, check_python_version(),
>                module_init)

Then it will get a warning: comparison between pointer and integer.

I think the benefit of code de-duplication is worth a complex marco. The
code using the macro won't get changed frequently so I won't worry too much
about it being used wrongly.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1 of 7 V2] util: add a macro initializing CPython modules

Yuya Nishihara
On Sat, 13 May 2017 21:08:59 -0700, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2017-05-14 12:57:57 +0900:
> > > +        (void)postinit; \
>
> This line could be changed to:
>  
>                if ((postinit) == -1) \
>                        return; \

Yea.

> > Maybe I'm a bit conservative about a macro use, but my concern here is,
> > it's so easy to make postinint never be called.
> >
> >   PYMODULEINIT(parsers, methods, parsers_doc, 1, check_python_version(),
> >                module_init)
>
> Then it will get a warning: comparison between pointer and integer.
>
> I think the benefit of code de-duplication is worth a complex marco. The
> code using the macro won't get changed frequently so I won't worry too much
> about it being used wrongly.

That could be a counter argument. Since it's a boilerplate code we won't
change too often, code duplication is just okay. That's my opinion.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1 of 7 V2] util: add a macro initializing CPython modules

Gregory Szorc
On Sat, May 13, 2017 at 9:44 PM, Yuya Nishihara <[hidden email]> wrote:
On Sat, 13 May 2017 21:08:59 -0700, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2017-05-14 12:57:57 +0900:
> > > +        (void)postinit; \
>
> This line could be changed to:
>
>                if ((postinit) == -1) \
>                        return; \

Yea.

> > Maybe I'm a bit conservative about a macro use, but my concern here is,
> > it's so easy to make postinint never be called.
> >
> >   PYMODULEINIT(parsers, methods, parsers_doc, 1, check_python_version(),
> >                module_init)
>
> Then it will get a warning: comparison between pointer and integer.
>
> I think the benefit of code de-duplication is worth a complex marco. The
> code using the macro won't get changed frequently so I won't worry too much
> about it being used wrongly.

That could be a counter argument. Since it's a boilerplate code we won't
change too often, code duplication is just okay. That's my opinion.

The duplicate code for module init already exists. This series just adds to it.

I'm fine with either continuing to dupe minor things, including adding a version check. Or, we can add a macro for just the version checking and call it 2x. Or, we can establish a convention for defining a "postinit" function per module, invoke that 2x as part of the boilerplate, and have each module's implementation call a helper or macro to do version checking.

Let's not make perfect the enemy of good.

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1 of 7 V2] util: add a macro initializing CPython modules

Jun Wu
Excerpts from Gregory Szorc's message of 2017-05-18 15:14:45 -0700:
> The duplicate code for module init already exists. This series just adds to
> it.

Previously there are 6 places with the duplication. Now it's just 1.

> I'm fine with either continuing to dupe minor things, including adding a
> version check. Or, we can add a macro for just the version checking and
> call it 2x. Or, we can establish a convention for defining a "postinit"
> function per module, invoke that 2x as part of the boilerplate, and have
> each module's implementation call a helper or macro to do version checking.
>
> Let's not make perfect the enemy of good.

To make progress, I'll drop this series from patchwork and revive my very
first series with "_version" changed to "version".
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel