Repository navigation
More readable module constant addition statement #127435
Description
Feature or enhancement
Proposal:
I would like to ask if we need to package the addition of macro and constant for some modules in Modules/*? I think they should be like socketmodule.c, maybe it will be better. If allowed, I'll submit the PR. Like this:
#define ADD_INT_MACRO(MOD, INT) do { \
if (PyModule_AddIntConstant(MOD, #INT, INT) < 0) { \
goto error; \
} \
} while (0)
#ifdef AF_APPLETALK
/* Appletalk DDP */
ADD_INT_MACRO(m, AF_APPLETALK);
#endif
for example:
from posixmodule.c
#ifdef TMP_MAX
if (PyModule_AddIntMacro(m, TMP_MAX)) return -1;
#endif
#ifdef WCONTINUED
if (PyModule_AddIntMacro(m, WCONTINUED)) return -1;
#endif
from symtablemodule.c
if (PyModule_AddIntMacro(m, USE) < 0) return -1;
if (PyModule_AddIntMacro(m, DEF_GLOBAL) < 0) return -1;
I'm not sure if this is a cosmetic change, but I think it's necessary since there are only a few old modules that don't have this.
Has this already been discussed elsewhere?
No response given
Links to previous discussion of this feature:
No response
Activity
rruuaanng commentedon Nov 30, 2024
cc @vstinner
picnixz commentedon Nov 30, 2024
We tend to avoid packaging macros like that, except for the C API tests which has some macros or hashlib-related code which also some macros. It would be better to have an ERRACTION suite that would be executed on error instead of just jumping to an 'error' label which may or may not exist. But honestly, considering the number of modules that would be affected and the additional indirection that it could cause while reading code, I doubt it would be accepted.
EDIT: Maybe I misunderstood, but do you mean to add modules-dedicated macros? if so, I think this would be a cosmetic-only change which also adds a level of indirection. The current code is fine IMO, and it could be refactored if we were to change the code in the future, but I don't think we'll just change the code for that.
EDIT 2: It might be ok for modules that have lots of constants just to improve readability. I would also ask @encukou, @erlend-aasland and @serhiy-storchaka since they are also part of the C API WG and may have a different opinion on that matter.
serhiy-storchaka commentedon Nov 30, 2024
I do not see how this is anything but cosmetic changes. Cosmetic changes are allowed if the maintainer of that code decided that this will make the future maintenance easier. For every case it should be decided separately. We should not take a solution and look for problems, we should look for a suitable solution for each problem.
rruuaanng commentedon Nov 30, 2024
I see. I'll package all unpackaged statements in all modules today. Stay tuned :)
picnixz commentedon Nov 30, 2024
I'm not sure that we decided to do this. Please, ask the maintainer of each module you want to change whether this is needed or not before creating a corresponding PR.
rruuaanng commentedon Nov 30, 2024
I agree that I'll list all the modules that need to be modified and complete them as required.
serhiy-storchaka commentedon Nov 30, 2024
I a maintainer of some module need this, they can change the code of the corresponding module. Currently there is no need to massrewrite all modules.
See also an alternative idea in #86542.
rruuaanng commentedon Nov 30, 2024
I'll read it carefully today and try to realize it in the near future.
Edit
In addition, thank you for your suggestions on this matter. I will pay close attention to the capi working group in the future.