Repository navigation
Convert static initializers of PyTypeObjects to use C99 Designated (named) initializers #127679
Description
Activity
I've been using the C99 named initializers consistently in all my module isolation changes (
PyType_Specstructs,PyModuleDefstruct, etc.). +1 for more readable and maintainable code.- Question 1: which fields should be top-level? (for contributors, this could be good to know in advance)
- Question 2: do you want one huge PR for all types?
- Question 3: do you want a comment separating required fields from optional ones?
META: this would have been a perfect fit for a
type-refactoringlabel (we've talked about such a label before)Reacted by Bénédikt Tran, Mark Shannon and Kirill PodoprigoraTBH, I was just using that layout as an example of how C99 can lead to more readable code.
I don't have strong opinions on what the order should be. The important thing is that changing the order doesn't break code.Reacted by Erlend E. Aasland and Bénédikt TranPersonally, I like when the code looks uniform and nice (call me a purist if you want). I think separating required from optional fields is good but sometimes, some fields are required (e.g.,
tp_traversewhen supporting the GC protocol).So it's good to know that by just looking at the code instead of rediving into the docs and the specs.
I prefer separating stuff like the number protocol and the sequence protocol. For
tp_traverse, I'd stick to just placing it neartp_freeandtp_clear.Reacted by Bénédikt TranQuestion 2: do you want one huge PR for all types?
My 2 øre: I'd split this up in one PR for each dir (most of these static types are found in Python/ or Objects/).
Reacted by Bénédikt TranI was reluctant to do such change, but I think that earlier or later we should do this. The end result is not just more readable, it is more greppable, and I often search in the code. But please open a topic on d.p.o and wait for a week. Otherwise, it could lead to a post-merge uproar, and possibly a rollback.
As for the order, just use the current order (possibly with few exceptions if there are reasons). One huge PR is okay. If you can make it (half-)automatically generated to reduce human factor -- even better.
Reacted by Kirill Podoprigora and Erlend E. AaslandI agree using named initializers would be a readability, grepability, and maintenance improvement.
I think it's better to only change code when you're touching it for other reasons.
But, especially if this is important enough that it should be done now, could you add it to PEP 7 first?
Reacted by Eric V. SmithI found some motivation for fixing #111178, and this leads me to changing a lot of code where I can encounter static exported types (e.g.: #128241 (comment)). Should I still wait for a DPO thread and/or PEP-7 update or can I make the changes in those files (only if I touch the structures)?
I think I'll wait for a consensus to be reached or at least for the following points to be addressed:
- decide to open a DPO thread and wait for a week (@serhiy-storchaka is this a required condition for you? I can find the motivation to write something tomorrow though)
- update PEP-7 (https://github.com/python/peps)
Reacted by Bogdan Romanyuk and Erlend E. Aasland
Named initializer are both easier to read and less error prone. There is AFAICT no downside to using them.
However, we have been historically reluctant to change code for purely maintenance. Personally, I think this leads to poorer code quality, but others value cleaner code history more.
In this case I think the readability and correctness advantages are such that we should make this change and accept the inconvenience in version control history.
As a motivating example, take the
booltype,PyBool_Type:There are a lot of zeros, and a lot of comments to make sure we have the right number of zeros. If one is missing, things go wrong.
Contrast that with the C99 version:
The code is self describing (no need for comments naming the fields) and is much less error prone. The fields can listed in any order, in this case with the core field first, then the optional fields alphabetically.