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