Patchwork [1,of,7] util: add a macro initializing CPython modules

login
register
mail settings
Submitter Jun Wu
Date May 9, 2017, 1:07 a.m.
Message ID <a2c5e183cafca6d58a0d.1494292042@x1c>
Download mbox | patch
Permalink /patch/20531/
State Superseded
Headers show

Comments

Jun Wu - May 9, 2017, 1:07 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1494288921 25200
#      Mon May 08 17:15:21 2017 -0700
# Node ID a2c5e183cafca6d58a0dd986870ac620be1fb107
# Parent  52ec3072fe46bc4b193b6273357e3cc40b4421ad
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r a2c5e183cafc
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.
Yuya Nishihara - May 13, 2017, 1:45 p.m.
On Mon, 8 May 2017 18:07:22 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1494288921 25200
> #      Mon May 08 17:15:21 2017 -0700
> # Node ID a2c5e183cafca6d58a0dd986870ac620be1fb107
> # Parent  52ec3072fe46bc4b193b6273357e3cc40b4421ad
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r a2c5e183cafc
> util: add a macro initializing CPython modules

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

I got lots of warnings because of the precheck/postinit expansion:

  mercurial/util.h:80:16: warning: the comparison will always evaluate as
  'true' for the address of 'module_init' will never be NULL [-Waddress]

> +			((int (*)(PyObject *))postinit)(m); \

It's dangerous to disable compiler type checking.

Overall, I prefer not using a big macro like this even though it could get
rid of some redundant codes.

Adding "version" constants seems good to me.
Jun Wu - May 13, 2017, 6:14 p.m.
Excerpts from Yuya Nishihara's message of 2017-05-13 22:45:14 +0900:
> I got lots of warnings because of the precheck/postinit expansion:
> 
>   mercurial/util.h:80:16: warning: the comparison will always evaluate as
>   'true' for the address of 'module_init' will never be NULL [-Waddress]
> 
> > +            ((int (*)(PyObject *))postinit)(m); \
> 
> It's dangerous to disable compiler type checking.
> 
> Overall, I prefer not using a big macro like this even though it could get
> rid of some redundant codes.

This is mainly responding to Greg's comment about code duplication.
Let me try if I can keep compiler type checking and remove those warnings.

If I can do that, I think the approach is still better - it avoids
unintentional Py2 and Py3 inconsistence. For example, mpatchError is exposed
in Py3, but not Py2 in current code.

> Adding "version" constants seems good to me.
Gregory Szorc - May 13, 2017, 8:48 p.m.
On Sat, May 13, 2017 at 11:14 AM, Jun Wu <quark@fb.com> wrote:

> Excerpts from Yuya Nishihara's message of 2017-05-13 22:45:14 +0900:
> > I got lots of warnings because of the precheck/postinit expansion:
> >
> >   mercurial/util.h:80:16: warning: the comparison will always evaluate as
> >   'true' for the address of 'module_init' will never be NULL [-Waddress]
> >
> > > +            ((int (*)(PyObject *))postinit)(m); \
> >
> > It's dangerous to disable compiler type checking.
> >
> > Overall, I prefer not using a big macro like this even though it could
> get
> > rid of some redundant codes.
>
> This is mainly responding to Greg's comment about code duplication.
> Let me try if I can keep compiler type checking and remove those warnings.
>

FWIW, I've been thinking about merging the C modules into a single one. IMO
there aren't many benefits to having separate C modules other than clearer
naming. Having all the code in a single compilation unit facilitates more
code reuse, more aggressive compiler optimization, etc. It would get a
little weird on the pure Python / CFFI side of the world.


>
> If I can do that, I think the approach is still better - it avoids
> unintentional Py2 and Py3 inconsistence. For example, mpatchError is
> exposed
> in Py3, but not Py2 in current code.
>
> > Adding "version" constants seems good to me.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Jun Wu - May 13, 2017, 11:10 p.m.
Excerpts from Gregory Szorc's message of 2017-05-13 13:48:18 -0700:
> FWIW, I've been thinking about merging the C modules into a single one. IMO
> there aren't many benefits to having separate C modules other than clearer
> naming. Having all the code in a single compilation unit facilitates more
> code reuse, more aggressive compiler optimization, etc. It would get a
> little weird on the pure Python / CFFI side of the world.

Moving C modules to a single file does not solve the Py2 / Py3 duplication
issue?

I have sent a V2 which solves the duplication and compiler warning issue.
Gregory Szorc - May 13, 2017, 11:26 p.m.
On Sat, May 13, 2017 at 4:10 PM, Jun Wu <quark@fb.com> wrote:

> Excerpts from Gregory Szorc's message of 2017-05-13 13:48:18 -0700:
> > FWIW, I've been thinking about merging the C modules into a single one.
> IMO
> > there aren't many benefits to having separate C modules other than
> clearer
> > naming. Having all the code in a single compilation unit facilitates more
> > code reuse, more aggressive compiler optimization, etc. It would get a
> > little weird on the pure Python / CFFI side of the world.
>
> Moving C modules to a single file does not solve the Py2 / Py3 duplication
> issue?
>

No. But at least you only have to write the module init code once instead
of N times. That makes it tolerable.


>
> I have sent a V2 which solves the duplication and compiler warning issue.
>
Yuya Nishihara - May 14, 2017, 2:54 a.m.
On Sat, 13 May 2017 13:48:18 -0700, Gregory Szorc wrote:
> On Sat, May 13, 2017 at 11:14 AM, Jun Wu <quark@fb.com> wrote:
> 
> > Excerpts from Yuya Nishihara's message of 2017-05-13 22:45:14 +0900:
> > > I got lots of warnings because of the precheck/postinit expansion:
> > >
> > >   mercurial/util.h:80:16: warning: the comparison will always evaluate as
> > >   'true' for the address of 'module_init' will never be NULL [-Waddress]
> > >
> > > > +            ((int (*)(PyObject *))postinit)(m); \
> > >
> > > It's dangerous to disable compiler type checking.
> > >
> > > Overall, I prefer not using a big macro like this even though it could
> > get
> > > rid of some redundant codes.
> >
> > This is mainly responding to Greg's comment about code duplication.
> > Let me try if I can keep compiler type checking and remove those warnings.
> >
> 
> FWIW, I've been thinking about merging the C modules into a single one. IMO
> there aren't many benefits to having separate C modules other than clearer
> naming. Having all the code in a single compilation unit facilitates more
> code reuse, more aggressive compiler optimization, etc. It would get a
> little weird on the pure Python / CFFI side of the world.

Merging C modules would be likely to create more import cycles, seen in
encoding.py for example.

Patch

diff --git a/mercurial/util.h b/mercurial/util.h
--- a/mercurial/util.h
+++ b/mercurial/util.h
@@ -43,3 +43,43 @@  typedef unsigned char bool;
 #endif
 
+#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 && ((int (*)(void))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 && ((int (*)(PyObject *))postinit)(m) == -1) \
+			return NULL; \
+		return m; \
+	}
+#else
+#define PYMODULEINIT(name, methods, doc, version, precheck, postinit) \
+	PyMODINIT_FUNC (init ## name)(void) \
+	{ \
+		PyObject *m; \
+		if (precheck && ((int (*)(void))precheck)() == -1) \
+			return; \
+		m = Py_InitModule3(#name, methods, doc); \
+		if (m == NULL) \
+			return; \
+		if (PyModule_AddIntConstant(m, "version", version) == -1) \
+			return; \
+		if (postinit != NULL) \
+			((int (*)(PyObject *))postinit)(m); \
+	}
+#endif
+
 #endif /* _HG_UTIL_H_ */