This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: _pickle.c structure cleanup
Type: enhancement Stage:
Components: Extension Modules Versions: Python 3.3
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: alexandre.vassalotti, loewis, pitrou, valhallasw
Priority: low Keywords:

Created on 2012-02-24 23:38 by valhallasw, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Repositories containing patches
https://bitbucket.org/valhallasw/cpython#_pickle_clean
Messages (6)
msg154165 - (view) Author: Merlijn van Deen (valhallasw) * Date: 2012-02-24 23:38
While working on #6784, I've looked at _pickle.c and found it quite... daunting: 6500 lines and 185kB. I have been working on a bit of cleanup, and I'd like some comments on this.

I'm working on adapting
_pickle.c into the following structure:

_pickle.c takes care of the module initialization
_pickle/*.c are helper files for this (global functions/definitions)
_pickle/PicklerObject contains all files that relate to the Pickler
class - initialization, all functions, etc, and
_pickle/PicklerObject/picklers/*.c contains all pickling functions,
split out into groups (corresponding to pickletools.StackObjects)

This should end in a tree where every .c module lists the relevant dependencies (and as such should be compilable on itself).

Currently, I'm at the point where PicklerObject roughly has the structure I want (although not every file is compilable on itself yet). As such, I feel this is the right moment to ask if it would be seen as an useful improvement in trunk, and to see if anyone has suggestions for improvements.

hg repos: https://bitbucket.org/valhallasw/cpython/src/0810ffadffa3/Modules/_pickle/PicklerObject (_pickle_clean branch)
msg154167 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-24 23:52
I think this is going overboard. _pickle.c is long but it defines two classes which are closely related to each other. I don't really get the point of exploding it into a myriad of 30-line files, especially if it means I now have to keep all these tiny files open when I want to change something in the pickle module.

If you want to find a target for modularizing, I think posixmodule.c would be a far better candidate.
msg154171 - (view) Author: Merlijn van Deen (valhallasw) * Date: 2012-02-25 00:10
That makes sense. The goal was not so much cleaning up the module per se; rather, it was a result of trying to understand the general structure of _pickler.c specifically.

However, is there an intermediate level of 'modularization' you would propose? The idea is of course not to have 30 files open to make a small change, but rather have one or two reasonably small files open. My current approach was fine-grained on purpose - it's not that much work to recombine files.

I think it makes sense to split the PicklerObject into several parts:
  * the Pickler object (initialization/type/etc, maybe functions split off)
  * the actualy pickler implementation (dump/save/save_*) (but maybe in less files - see below)
  * the PicklerMemoProxy

for the picker implementation - specifically the picklers directory - my approach was to have a 'mirrored' directory for PicklerObject and UnPicklerObject: the methods to pickle and unpickle will be in the same files in the two directories (i.e. picklers/unicode.c will pickle str, unpicklers/unicode.c will unpickle them).
msg154233 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-02-25 10:28
I'm -1 on splitting the file. This is C, splitting it up will make it *harder* to understand, as you have to search across multiple files to find anything.

If you want to make it more readable, I propose that you
a) put a comment in the top explaining how the file is structured
b) put

/*****************************************************************/

  section headers between logical groups of functions.
msg154237 - (view) Author: Merlijn van Deen (valhallasw) * Date: 2012-02-25 11:04
See https://bitbucket.org/valhallasw/cpython/src/ee0d2beaf6a4/Modules/_pickle.c for a rough structure overview - which maybe also explains why I thought restructuring made sense in the first place.

However, I'm not the person who has to maintain the module. If you're not interested, I'll just split out the module until I feel comfortable editing stuff, make my patch for #6784 and backport it to the 6500-line version.
msg154278 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-02-25 18:16
Thanks for the effort, but I'm rejecting this now. I'm not fundamentally opposed to restructing this code, but I do think that your change would be a slight loss of readability. If you absolutely cannot stand working with such a large code, please try to find supporters of a change on python-dev.

Wrt. your actual approach: this is somewhat uncoommon. If you have many files, you'd rather expect them to be integrated in the build process (i.e. with Makefile and VS project file changes) rather than recursive includes.
History
Date User Action Args
2022-04-11 14:57:27adminsetgithub: 58326
2012-02-25 18:16:34loewissetstatus: open -> closed
resolution: rejected
messages: + msg154278
2012-02-25 11:04:16valhallaswsetmessages: + msg154237
2012-02-25 10:28:58loewissetnosy: + loewis
messages: + msg154233
2012-02-25 00:10:06valhallaswsetmessages: + msg154171
2012-02-24 23:52:47pitrousetpriority: normal -> low
versions: - Python 3.4
nosy: + alexandre.vassalotti, pitrou

messages: + msg154167
2012-02-24 23:38:05valhallaswcreate