Troy Curtis Jr wrote on Sat, 23 Dec 2017 15:27 +0000:
> On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <d.s_at_daniel.shahaf.name>
> wrote:
>
> > troycurtisjr_at_apache.org wrote on Sat, 23 Dec 2017 04:43 +0000:
> > > Author: troycurtisjr
> > > Date: Sat Dec 23 04:43:26 2017
> > > New Revision: 1819110
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1819110&view=rev
> > > Log:
> > > On branch swig-py3: Replace hasattr check for a method with try-except.
> > >
> > > * subversion/bindings/swig/include/proxy.swg
> > > (_assert_valid_deep): Replace hasattr check for the 'assert_valid'
> > method
> > > with a try-except block as some class instances can have the method
> > but return
> > > False to hasattr().
> >
> > I'm not too familiar with this code; shouldn't we be fixing the original
> > problem, of hasattr() wrongly returning False? I assume it predates the
> > branch, though?
> >
> >
> I won't lie, I didn't like this change much, since I didn't feel that I
> understood exactly *why* it didn't work. I only found info stating that
> hasattr effectively did a getattr, but translated the AttributeError into a
> boolean. However, obviously *something* else is different. The attribute
> is obviously able to be found in some scenarios, but returns false for
> hasattr. So far my attempts to reproduce in a small test class haven't
> been successful. Perhaps, I should continue to dig into this one to get to
> the bottom of what the difference actually is.
I assume it could affect users' code as well, so yes, it'll be nice to get to
the bottom of it (and to confirm that it's not a regression). What classes can
you reproduce the mismatch with?
I don't see any difference between the CPython implementations of getattr and
hasattr, but perhaps I'm overlooking something (or looking at the source of a
different CPython version to yours).
>
> > While reviewing this I also noticed that svn_swig_py_convert_ptr() also
> > does this hasattr() check — not sure whether it needs to change too? —
> > and also has an "x | 0" construct, which seems suspicious (isn't it a
> > no-op?).
> >
> >
> I looked back through the logs in an attempt figure out the rationale for
> the "x | 0" construct, but it just shows up that way when it was first
> added [0]. I agree, it should just be changed to the define. I can't come
> up with a reason to 'or' with 0.
SWIG_POINTER_EXCEPTION is defined as «0» in swig3 so the construct does appear
to be a no-op there. I don't have older swigs to test with.
(@brane: Good catch.)
> 0: svn diff -r855533:855534
> ^/subversion/branches/python-bindings-improvements/subversion/bindings/
> swig/python/libsvn_swig_py/swigutil_py.c_at_855534
Also known as
https://svn.apache.org/viewvc/subversion/branches/python-bindings-improvements/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c?r1=855534&r2=855533&pathrev=855534
Cheers,
Daniel
Received on 2017-12-23 21:57:13 CET