Bazaar

Code review comment for lp:~jameinel/bzr/2.1-static-tuple-btree-string-intern

2.1-static-tuple-btree-string-intern
Merge into bzr.dev

Revision history for this message

John A Meinel (jameinel) wrote on 2009-10-14:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Review: Approve
> 384 + refs_as_tuples = tuple([tuple([tuple(ref) for ref in ref_list])
> 385 + for ref_list in node[3]])
>
> I wonder if it would be worth adding a convenience method, perhaps StaticTuple.as_tuples(), that recursively does this conversion. That would make this ugliness unnecessary.

As for "as_tuples()" I would be fine just extending ".as_tuple()" to do
exactly that. The main restriction is that we may not always have tuples
at this point.

At least so far, tuple is interchangeable w/ StaticTuple.

>
> 431 + # I don't believe we can define a method by which
> 432 + # (prefix,) + StaticTuple will work, though we could
>
> In plain Python you could define an __radd__ for this, so surely there's a way to do this in C?
>
> class T(object):
> def __radd__(self, other):
> return 'haha!'
> t = T()
>
> print ('tuple',) + t # prints 'haha!'
>

Tuple uses "tp_as_sequence.sq_concat" to handle ('tuple',) + t, which I
didn't think worked the other way. But thanks for pointing me to this,
I'll look into it.

> You may need to do something odd like provide the nb_add slot, even though this isn't really a numeric type, but I think that's ok. (All pure python classes would have that I think, even the non-numeric ones, so presumably having tp_as_number filled doesn't automatically make Python do dumb things.)
>
> I think we can live without this, but it would be nice.

Actually, the main reason I added the comment is because I expect things
to fail at that point, but I haven't gotten a test case to trigger it,
and it also won't trigger with --2a formats... (They don't have missing
compression parents.)

>
> 488 + k1 = stuple(stuple('<email address hidden>',),
> 489 + stuple('<email address hidden>',))
> 490 + k2 = stuple(stuple(stuple('<email address hidden>',),
> 491 + stuple('<email address hidden>',)),
> 492 + stuple('<email address hidden>',))
>
> This test data is needlessly complex and hard to read. Why not e.g.:
>
> k1 = stuple(stuple('a',), stuple('b',))
> k2 = stuple(stuple(stuple('c',), stuple('d',)), stuple('a',))
>
> Which is structurally the same and much easier to follow.

Sure. I did the above because that was the actual data I was getting. Of
course, I've since narrowed it down to a bug in interning....

Anyway, I'm happy to simplify it, and should have done so before submitting.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrVNP8ACgkQJdeBCYSNAAN61QCbBceibTUybQ2cwzsABrC2rcPc
RwcAni/o9YUyAE/7ShfvcoeHZFGUMwDw
=ZhaX
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Review: Approve
> 384	+ refs_as_tuples = tuple([tuple([tuple(ref) for ref in ref_list])
> 385	+ for ref_list in node[3]])
> 
> I wonder if it would be worth adding a convenience method, perhaps StaticTuple.as_tuples(), that recursively does this conversion.  That would make this ugliness unnecessary.

As for "as_tuples()" I would be fine just extending ".as_tuple()" to do
exactly that. The main restriction is that we may not always have tuples
at this point.

At least so far, tuple is interchangeable w/ StaticTuple.

> 
> 431	+ # I don't believe we can define a method by which
> 432	+ # (prefix,) + StaticTuple will work, though we could
> 
> In plain Python you could define an __radd__ for this, so surely there's a way to do this in C?
> 
> class T(object):
>     def __radd__(self, other):
>         return 'haha!'
> t = T()
> 
> print ('tuple',) + t  # prints 'haha!'
>

Tuple uses "tp_as_sequence.sq_concat" to handle ('tuple',) + t, which I
didn't think worked the other way. But thanks for pointing me to this,
I'll look into it.

> You may need to do something odd like provide the nb_add slot, even though this isn't really a numeric type, but I think that's ok.  (All pure python classes would have that I think, even the non-numeric ones, so presumably having tp_as_number filled doesn't automatically make Python do dumb things.)
> 
> I think we can live without this, but it would be nice.

> 
> 488	+ k1 = stuple(stuple('launchpad@pqm.canonical.com-20080513123945-5rksdb3kzaszgujl',),
> 489	+ stuple('bjorn@canonical.com-20080513090729-gxckcc7gv01txwl6',))
> 490	+ k2 = stuple(stuple(stuple('tim.penhey@canonical.com-20080805095941-04dh2yw94fjtialz',),
> 491	+ stuple('launchpad@pqm.canonical.com-20080805101011-jt59xexr9sdkcjiu',)),
> 492	+ stuple('bjorn@canonical.com-20080513090729-gxckcc7gv01txwl6',))
> 
> This test data is needlessly complex and hard to read.  Why not e.g.:
> 
>     k1 = stuple(stuple('a',), stuple('b',))
>     k2 = stuple(stuple(stuple('c',), stuple('d',)), stuple('a',))
> 
> Which is structurally the same and much easier to follow.

Sure. I did the above because that was the actual data I was getting. Of
course, I've since narrowed it down to a bug in interning....

iEYEARECAAYFAkrVNP8ACgkQJdeBCYSNAAN61QCbBceibTUybQ2cwzsABrC2rcPc
RwcAni/o9YUyAE/7ShfvcoeHZFGUMwDw
=ZhaX
-----END PGP SIGNATURE-----

« Back to merge proposal