On 28 February 2012 21:38, Stefan Behnel <[email protected]> wrote: > mark florisson, 28.02.2012 22:19: >> On 28 February 2012 21:08, Stefan Behnel wrote: >>> mark florisson, 28.02.2012 21:20: >>>> On 28 February 2012 19:58, Stefan Behnel wrote: >>>>> mark florisson, 28.02.2012 16:35: >>>>>> Basically, the cleanup code only needs a matching release because the >>>>>> corresponding acquire is in EnsureGILNode, which wraps the function >>>>>> body in case of a nogil function with a 'with gil' block. Any changes >>>>>> to the conditions in FuncDefNode will have to be reflected by the code >>>>>> that does that wrapping. Changing the refnanny macro for the cleanup >>>>>> code will not avail anything, as the GIL is already ensured. >>>>> >>>>> Regarding the "with gil" code, ISTM that the "finally" code in the >>>>> with_gil >>>>> test is being duplicated. I noticed this when I moved the refnanny's GIL >>>>> state into a block local variable and that broke the C code. Basically, >>>>> the >>>>> with-gil block had declared the variable in its own block, but was then >>>>> trying to access that variable in a second finally clause, further down >>>>> and >>>>> outside of the with-gil block. >>>>> >>>>> Looks like a bug to me. >>>> >>>> That's not a bug, that's how it is implemented. At setup, a variable >>>> __pyx_gilstate_save is declared, which is also used for teardown. Any >>>> with GIL blocks use C block scoping to do the same thing. The with >>>> block is itself a try/finally, so you get a try/finally wrapped in a >>>> try/finally. The code uses try/finally for it's way of trapping >>>> control flow, allowing some code to execute and resuming control flow >>>> afterwards. >>> >>> Ok, so what really got me confused is that the code used the variable >>> "acquire_gil_for_refnanny_only" in places that didn't have anything to do >>> with the refnanny. Similarly, "acquire_gil_for_var_decls_only" was used for >>> cleanup even though the GIL had already been released if this flag was set, >>> way before the end of the function. >>> >>> I think I fixed both issues in the patch I attached. At least, it still >>> passes the gil related tests and doesn't raise any C compiler warnings >>> about the GIL state variable being unused. >>> >>> Does this look about right? >> >> It looks right to me, yeah. > > Ok. I think this looks simple enough to go into the release, whereas any > more advanced cleanup and optimisations should have their time to mature. > Would you object? >
Sure, that's fine with me. >> (<pedantic>I prefer to format bools >> directly with %d, as they are a subclass of int anyway</pedantic>). > > I don't. It works when they are really booleans, but in Python, many things > that are "true" or "false" aren't actually of type bool. When it comes to > writing out (user visible) data, it's always best to make it clear what the > intended output is, instead of relying on 'implementation details' and data > of unknown sources (or different purposes, as in this case). > > >> I think in general the EnsureGILNode should have been mentioned in the >> code generation function of FuncDefNode, which makes it easier to >> figure out what is going on. The documentation is currently at the >> wrapping site in AnalyseDeclarationsTransform. > > I'll add a comment. > > Stefan > _______________________________________________ > cython-devel mailing list > [email protected] > http://mail.python.org/mailman/listinfo/cython-devel _______________________________________________ cython-devel mailing list [email protected] http://mail.python.org/mailman/listinfo/cython-devel
