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

login
register
mail settings
Submitter Jun Wu
Date May 13, 2017, 6:55 p.m.
Message ID <55ae1324c950e76270cc.1494701723@x1c>
Download mbox | patch
Permalink /patch/20604/
State Superseded
Headers show

Comments

Jun Wu - May 13, 2017, 6:55 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# 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.
Yuya Nishihara - May 14, 2017, 3:57 a.m.
On Sat, 13 May 2017 11:55:23 -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 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)
Jun Wu - May 14, 2017, 4:08 a.m.
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.
Yuya Nishihara - May 14, 2017, 4:44 a.m.
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.
Gregory Szorc - May 18, 2017, 10:14 p.m.
On Sat, May 13, 2017 at 9:44 PM, Yuya Nishihara <yuya@tcha.org> 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.
Jun Wu - May 19, 2017, 3:45 p.m.
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".

Patch

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_ */