Bad code in bzrlib.inventory

[John Arbash Meinel]

Jan Hudec wrote:
> Hello,
> 
> I have noticed a bad code in bzrlib/inventory.py, that only works by luck:
> 
> 1. class Inventory defines __slots__ that do NOT include symlink_target.
> 2. It's __init__ ASSIGNS self.symlink_target
> 3. It's __eq__ COMPARES self.symlink_target
> That's bad, right?
> 4. class RootEntry does not define __slots__, which gives it a __dict__
> 5. class InventoryDir does not define __slots__, so again has __dict__
> 6. class InventoryFile does not define __slots__, so again has __dict__
> 7. class InventoryLink defines __slots__ = ['symlink_target']
> So all the actually instantiated classes either have dict (wasn't the point
> of using __slots__ to avoid allocating one?) or have that slot and therefore
> the references in the base class work. In other places (like the serializer)
> references to the attribute are made. But should it really work this way??
> 

Well, probably it works because we have a test suite which would fail if
it wasn't working. However, I agree that the code seems very bogus.

My suggestion is that everything which inherits from InventoryEntry
should use __slots__ (since we found a genuine performance improvement
when it was first introduced), and that if InventoryEntry.__eq__
compares the value, then it should be one of the slot members of
InventoryEntry.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060501/1ab11866/attachment.pgp