zopefoundation / Acquisition

Acquisition is a mechanism that allows objects to obtain attributes from the containment hierarchy they're in.
Other
12 stars 12 forks source link

C extension clean #16

Closed stephan-hof closed 7 years ago

stephan-hof commented 7 years ago

Give some love to _Acquistion.c

While porting this file to python3 I got the feeling that it needs some clean up. Some functions were very hard to read and this mix of tabs/spaces and missing brackets made it even harder to edit. So I started naively by "lets have a look what can be improved" and in the end hardly a line left unchanged.

The vast majority of changes are clean ups like.

The only functional change I did was to implement __new__ on a C level. This ensures that an empty wrapper is impossible to create. So as soon as you have a Wrapper object you can rely on self->obj being set. I cannot see an use case were an empty wrapper is desired. This change made it possible to simplify the code a lot. As far as I can see the python version is already doing it that way too: https://github.com/zopefoundation/Acquisition/blob/master/src/Acquisition/__init__.py#L361

My biggest fear are problems with refcount (=> memory leaks). So I applied this patch to the doctest module https://bugs.python.org/file10060/reset_globs.patch to use the --repeat and --report-refcounts of zope.testrunner.

Repeating the tests several 1000 times (it was running for 2 hours) did not show any growth of memory (RSS) nor refcounts. So I feel pretty safe here.

I also used this branch to run the unittests of Zope and everything is OK there too.

There are also changes which should fix potential segfaults / memory leaks. https://github.com/zopefoundation/Acquisition/commit/12c446c2051d1be000e34467d612c297f93ec62f https://github.com/zopefoundation/Acquisition/commit/acaee010e0e4778559b483f06fef4ad7f3efcef1

hannosch commented 7 years ago

Wow, you weren't kidding when you said you wanted to do "some" cleanup :))

I'll try to review this shortly, but might not get to it until the weekend.

One first complaint: I think there's still tabs in the untouched diff_to_bool function ;)

hannosch commented 7 years ago

Finally managed to review this. I'm 95% certain this is correct. Given that all tests pass and you did the repeated tests for memory / refcount leaks, I'd say this is all good.