On Sat, Dec 23, 2017 at 2:57 PM Daniel Shahaf <d...@daniel.shahaf.name> wrote:
> Troy Curtis Jr wrote on Sat, 23 Dec 2017 15:27 +0000: > > On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <d...@daniel.shahaf.name> > > wrote: > > > > > troycurti...@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). > > The place I was finding the issue was on a SWIG wrapped auth baton instance used in a client context. In particular, in bindings/swig/python/tests/pool.py:87 during the test_assert_valid function, the assert_valid on the auth_baton instance would fail. When viewed in debugger, just before the hasattr() check, I confirmed that it returned 'False', but would actually run when called. > > > > > 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@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 >
