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