Re: [Jython-dev] [Jython-checkins] SF.net SVN: jython: [3871] branches/modern/src/org/python

[Charlie G. <cha...@gm...>]

I have a couple minor code style nits to pick with this commit...  Now
the fact that I'm raising these nits doesn't actually mean you should
do anything about them.  We don't actually have a standard way of
doing things in Jython, and everyone else may disagree with me.  I
figured it was a good idea to get them out in the open though so we
can come together on them a bit.

As a first order thing, I would've split this out into two commits.
Your dict changes could break stuff, so it's a good idea to get them
off on their own branch, but the itertools changes are pretty harmless
and might be nice for other things.  I would've committed those
directly to trunk so no one working on trunk and not paying attention
to your branch would notice the absence of tee and work on adding it.

On Dec 26, 2007 10:47 AM,  <zy...@us...> wrote:
> Revision: 3871
>           http://jython.svn.sourceforge.net/jython/?rev=3871&view=rev
> Author:   zyasoft
> Date:     2007-12-26 10:47:13 -0800 (Wed, 26 Dec 2007)
>
> Log Message:
> -----------
> Added itertools.tee (from 2.4). PyStringMap now accommodates null values, which is necessary for ClassDictInit-coded modules.
>
> Modified Paths:
> --------------
>     branches/modern/src/org/python/core/PyStringMap.java
>     branches/modern/src/org/python/modules/itertools.java
>
> Modified: branches/modern/src/org/python/core/PyStringMap.java
> ===================================================================
> --- branches/modern/src/org/python/core/PyStringMap.java        2007-12-26 17:52:00 UTC (rev 3870)
> +++ branches/modern/src/org/python/core/PyStringMap.java        2007-12-26 18:47:13 UTC (rev 3871)

[snip]

> +final class NullValue {
> +}
> +
>  public class PyStringMap extends PyObject
>  {
> -    private final Map table;
> +    private final ConcurrentMap table;
>
> +
> +    // CHM cannot store null values. Some Python code, specifically using
> +    // classDictInit components, assumes that this is possible to hide from
> +    // Python. So we have to compensate here.
> +    private final static NullValue nullValue = new NullValue();

Is there any reason for NullValue to be a class?  It seems like you're
just checking reference equals, so it could just be an instance of
Object.  If there isn't a reason it's a class, it just makes me wonder
about it every time I look at this code.  Actually, if all we're doing
is making sure it looks like nothing is in the dict when something
gets set to null, why can't we just delete the key from the dict when
setting a value to null?  That would save this extra nullable check
for every access to PyStringMap which is probably the most visited
piece of code in Jython.

[snip]

> +    // TODO: implement __copy__ protocol
> +    private final static class Tee {
> +
> +        private final PyObject iterator;
> +        private final ConcurrentMap buffer;
> +        private final int[] offsets;
> +        private TeeIterator[] tees;

Making things final is good, but if you do it, everything that can be
final should be final.  It looks like tees can be final too.  I have a
problem with wondering about the odd man out.  Also, since we've gone
to Java 1.5, you might as well use generics for buffer's contents and
eliminate a couple casts, and unless you're using specific features
from ConcurrentMap, it's always better to declare the least specific
interface you're using.  That'd be Map in this case.

[snip]

> +    /**
> +     * Create a tuple of iterators, each of which is effectively a copy of iterable.
> +     * @param iterable
> +     * @param n
> +     * @return PyTuple
> +     * @author Jim Baker <jb...@zy...>
> +     * @since 2.4
> +     */
> +    public static PyTuple tee(PyObject iterable, final int n) {
> +        return Tee.makeTees(iterable, n);
> +    }

The actual comment you wrote is good(except I might say "a tuple of n
iterators") since it says something, but the ones filled in by your
IDE don't add anything that isn't in the method signature, so they
should go.  I've also been getting rid of author attribution in actual
source code when I run across it for the reasons listed in
http://producingoss.com/producingoss.html#territoriality  The @since
doesn't make sense as there hasn't been a release of 2.3 for Jython
and there won't be a 2.4.  The whole module should probably be 2.5
since that's the first time it'll appear in Jython.

Charlie