On Tue, Feb 22, 2011 at 12:02 PM, W. Trevor King wrote:
> On Tue, Feb 22, 2011 at 08:18:21PM +0100, Stefan Behnel wrote:
>> W. Trevor King, 22.02.2011 18:55:
>> > I've been working on a more explicit parser that removes the ambiguity
>> > behind the various visibilities. This will help me ensure proper
>> > impolementation of my cdef-ed enums/structs/..., and make it easier to
>> > update visibility syntax in the future. Take a look and let me know
>> > if you think this is a useful direction to take:
>>
>> First thing that caught my eyes: I hope you do not intend to leave the
>> logging usage in the parser. This is seriously performance critical code
>> that Cython compiles down to pretty fast C code. Note that you can use a
>> debugger and Python's profiling/tracing interface to find out what's
>> happening, without code impact.
>
> I can strip them out afterwards, but it helps me figure out what I've
> broken if I shift too much around at the same time.
>
> I don't know enough about Python's trace module to know if I can turn
> on tracing only for functions defined in a single module or not, since
> otherwise its hard for me to separate signal from noise.
I think you can filter things after the fact. It would also be pretty
easy to write a utility that (conditionally) decorates all methods if
a flag is set, which we could leave in. (Wouldn't normally be such a
big deal, but this is one of the bottlenecks of compilation.)
>> Some of the log statements span more than one line, which makes it trickier
>> to strip them out with sed&friends (but backing out the initial changeset
>> would likely make it simple enough to remove the rest manually).
>
> Hmm, perhaps I'll condense the logging statements down onto one (long)
> line a piece, that will make it easy to comment/uncomment them with
> sed/emacs/etc. I suppose once Cython can compile the logging module
> we could leave them in with reduced overhead ;).
>
>> Also note that it's best to write runnable tests ("tests/run/"). The
>> tests in "tests/compile/" are only compiled and imported. See the
>> hacking guide in the wiki. I know you're not there yet with your
>> implementation, I'm just mentioning it.
>
> Thanks for the tip.
>
>> CtxAttribute is the wrong name, though. And the class copy
>> implementation gets even more generic than it already was in the
>> Ctx. I'm not a big fan of that, especially not in the parser. For
>> one, it prevents changing the classes into cdef classes, which had
>> long been on my list for Ctx.
>
> An easy, if uglier, workaround would be to prepend attributes with the
> class name, e.g. CBinding.visibility -> CBinding.c_binding_visiblity.
> Then the Ctx class could subclass the current CtxAttribute classes
> instead of binding instances of each of them. That way Ctx would keep
> its traditional flat attribute namespace and easy deepcopy, but
> eveyone in Nodes, etc. that will use the attributes would become
> class-name dependent.
I'd be up for flattening this. In particular, changing every
"entry.name" to "entry.python_binding.name" seems to be a lot of churn
and extra verbiage for not much benefit. The only overlap I see is
name and visibility, and keeping name/cname and adding cvisibility
would be preferable to me.
> The CtxAttribute class is, as its docstring says, just a hook for its
> deepcopy method. With an alternative deepcopy implementation,
> CtxAttribute could be replaced with the standard `object`, so don't
> worry too much about its name at this point ;).
You mean shallow copy?
>> CSource: doesn't sound like quite the right name - it does not describe a C
>> source file but information that Cython has about non-Cython things.
>
> It's a container for attributes that describe the presence and
> location of backing C definitions.
>
> * cdef: "Will there be a backing C defintion?
> * extern: "Has someone else already written it?"
> * name/namespace: "What did they call it?"
>
> If you'd rather I called the class something else, I'm certainly
> willing to change it.
It seems a bit odd to me, but if need be we can rename it later.
However, csource and c_binding seem rather redundant to me, but as
mentioned above I think it's better just to flatten it all.
The changes to parsing look decent to me, but admittedly there's a lot
of renaming churn, so I could have missed something.
- Robert
___
cython-devel mailing list
cython-devel@python.org
http://mail.python.org/mailman/listinfo/cython-devel
