[Patches] [ python-Patches-1623563 ] Allow class assignment for classes with slots

Thu Mar 15 23:56:55 CET 2007

Patches item #1623563, was opened at 2006-12-28 03:48
Message generated for change (Comment added) made by nnorwitz
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1623563&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Core (C code)
Group: Python 2.6
Status: Open
Resolution: Accepted
Priority: 5
Private: No
Submitted By: TH (therve)
Assigned to: Žiga Seilnacht (zseil)
Summary: Allow __class __ assignment for classes with __slots__

Initial Comment:
I made a modification in typeobject.c to allow __class__ modification for classes with slots. It's basically a change in same_slots_added to count the offset of the slots and check if the names of the slots are the same (in the naive way). 

I don't check if slots are in a different order, that may be an improve.

The patch is against trunk, with some tests. It's my first submission on Python, so every feedback will be welcome :).

----------------------------------------------------------------------

>Comment By: Neal Norwitz (nnorwitz)
Date: 2007-03-15 15:56

Message:
Logged In: YES 
user_id=33168
Originator: NO

Go ahead and check in.  Thanks!

----------------------------------------------------------------------

Comment By: Žiga Seilnacht (zseil)
Date: 2007-03-15 11:51

Message:
Logged In: YES 
user_id=1326842
Originator: NO

Hi Neal,

Yes, an exception in slots comparison would get
overwritten, but I think that this is highly
unlikely to happen. The user would have to put a
malicious string into __slots__, and that string
would have to go through list sorting in type_new()
without raising an error.

The original object is never stored in ht_slots;
type_new() always constructs a new tuple from the
sorted slot names.

I'll leave the patch open until tomorow in case you
have any other concerns, and then check it in.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2007-03-14 22:42

Message:
Logged In: YES 
user_id=33168
Originator: NO

Sorry about the typo, I did mean remove, not review.  The latest patch
looks good.  

I have some questions though.  What would happen if an exception is raised
when comparing slots?  I think it will be ok, the exception would get
overwritten with the one about incompatible types/__slots__.  

I just want to be safe and it might be good to at least test this
manually.  Is the original object saved in ht_slots or is it always a
tuple?  If the original object is used, what if you have two classes with
the same slots, but one uses a list and the other uses a tuple for
__slots__?

----------------------------------------------------------------------

Comment By: Žiga Seilnacht (zseil)
Date: 2007-03-14 07:24

Message:
Logged In: YES 
user_id=1326842
Originator: NO

The ht_slots tuple can't contain unicode names;
they are converted to strings in type_new(). type_new()
also raises an error for any non string object found in
__slots__. At the end of type_new(), ht_slots tuple can
only contain valid identifiers minus __weakref__ and
__dict__.

I changed the patch to use PyObject_Compare() anyway,
because it reduces the amount of code. I also removed
whitespace changes in the docs, changed the tests as
suggested and added a test with unicode slots.

File Added: same_slots_added_5.diff

----------------------------------------------------------------------

Comment By: TH (therve)
Date: 2007-03-14 04:22

Message:
Logged In: YES 
user_id=1038797
Originator: YES

> Ziga, there are a lot of changes to the doc that appear to be
whitespace
> only.  Could you review them from the patch before checkin?

I guess you mean "remove them" ?

> I see _PyString_Eq is used.  It's possible that __slots__ contain
unicode,
> possibly other types (I didn't check).

It seems __slots__ can be unicode, but only with ascii content. This
behavior is a bit weird...

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2007-03-13 21:01

Message:
Logged In: YES 
user_id=33168
Originator: NO

Ziga, there are a lot of changes to the doc that appear to be whitespace
only.  Could you review them from the patch before checkin?  If you want to
make whitespace/formatting changes, that's fine, but it's a lot easier to
review if there are 2 separate checkins.

In the test, it looks like it would be much easier to unpack the classes
in the loop, since you know there are two.  That would eliminate 2 of the
inner loops which don't seem necessary.

  for cls1, cls2 in ((G, H), (P, Q)):

Also, when doing data driven tests, it's important to add an error message
about which iteration failed.  Right now, you wouldn't know if G/H failed
or P/Q failed since the line number will be the same.  If you add a message
to the assertions which contains data, then it's clear from the message
which one failed.

typeobject.c

I see _PyString_Eq is used.  It's possible that __slots__ contain unicode,
possibly other types (I didn't check).  What happens if non-strings are in
__slots__ with this patch applied?

The calculation for size could be done outside the loop with a multiply. 
I don't know which would be faster.  I don't really care which is used
either.  Just something I noticed.

----------------------------------------------------------------------

Comment By: TH (therve)
Date: 2007-03-13 10:12

Message:
Logged In: YES 
user_id=1038797
Originator: YES

That's great, thanks a lot for you help.

----------------------------------------------------------------------

Comment By: Žiga Seilnacht (zseil)
Date: 2007-03-13 09:16

Message:
Logged In: YES 
user_id=1326842
Originator: NO

The only problem that I see with this patch is that
slot names have to be listed in both classes in the
same order for __class__ assignment to work. This
could have strange consequences when a dict is used
for __slots__.  

I'm attaching a patch that fixes this, with modified
test and documentation. The change had to be done
in type_new() function, by sorting slotnames before
the creation of member descriptors. Most of the
documentation changes are whitespace cleanup.

File Added: same_slots_added_4.diff

----------------------------------------------------------------------

Comment By: TH (therve)
Date: 2007-03-08 05:08

Message:
Logged In: YES 
user_id=1038797
Originator: YES

File Added: same_slots_added_3.diff

----------------------------------------------------------------------

Comment By: Žiga Seilnacht (zseil)
Date: 2007-03-07 09:07

Message:
Logged In: YES 
user_id=1326842
Originator: NO

The PyObject * cast is reduntant, but otherwise the
patch looks good to me. I added a few more tests,
but I can't attach a file so I posted them here:

http://freeweb.siol.net/chollus/

----------------------------------------------------------------------

Comment By: TH (therve)
Date: 2007-03-07 05:05

Message:
Logged In: YES 
user_id=1038797
Originator: YES

Thanks, I wasn't able to find this function.

I updated the patch for using this.
File Added: same_slots_added_2.diff

----------------------------------------------------------------------

Comment By: TH (therve)
Date: 2007-03-07 05:04

Message:
Logged In: YES 
user_id=1038797
Originator: YES

File Added: same_slots_added_2.diff

----------------------------------------------------------------------

Comment By: Žiga Seilnacht (zseil)
Date: 2007-03-06 10:07

Message:
Logged In: YES 
user_id=1326842
Originator: NO

You should use _PyString_Eq() for string comparison instead
of relying on the internal details of PyStringObject.

----------------------------------------------------------------------

Comment By: Jim Jewett (jimjjewett)
Date: 2007-02-08 10:41

Message:
Logged In: YES 
user_id=764593
Originator: NO

Review five other patches. 

Post the review summaries (and tracker numbers) to the python-dev mailing
list.

In that same message, ask someone with commit privileges to do the 5:1
deal, pointing at this tracker number.

----------------------------------------------------------------------

Comment By: TH (therve)
Date: 2007-02-06 05:43

Message:
Logged In: YES 
user_id=1038797
Originator: YES

Is there anything I can do to have a resolution on this ?

Thanks.

----------------------------------------------------------------------

Comment By: Jim Jewett (jimjjewett)
Date: 2006-12-30 16:26

Message:
Logged In: YES 
user_id=764593
Originator: NO

Just going through the list doesn't seem so naive to me.  If the slots are
in a different order, then you would need to move the data around -- which
borders on "maybe they ought to have written the translator themselves"

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1623563&group_id=5470

[Patches] [ python-Patches-1623563 ] Allow __class __ assignment for classes with __slots__

[Patches] [ python-Patches-1623563 ] Allow class assignment for classes with slots