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. > 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. > > Modified: > > > subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg > > > > Modified: subversion/branches/swig-py3/subversion/bindings/swig/include/ > > proxy.swg > > URL: > > > http://svn.apache.org/viewvc/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg?rev=1819110&r1=1819109&r2=1819110&view=diff > > > ============================================================================== > > --- subversion/branches/swig-py3/subversion/bindings/swig/include/ > > proxy.swg (original) > > +++ subversion/branches/swig-py3/subversion/bindings/swig/include/ > > proxy.swg Sat Dec 23 04:43:26 2017 > > @@ -57,8 +57,11 @@ > > _assert_valid_deep(v) > > # Ensure that the passed in value isn't a type, which could have an > > # assert_valid attribute, but it can not be called without an > > instance. > > - elif type(value) != type and hasattr(value, "assert_valid"): > > + elif type(value) != type: > > + try: > > value.assert_valid() > > + except AttributeError: > > + pass > > Hmm. Strictly speaking, the equivalent form would be: > > try: > value.assert_valid > except AttributeError: > pass > else: > value.assert_valid() > > since we don't want to mask any AttributeErrors inside assert_valid(). > I'm not sure how careful we are about this distinction, though. > > Good catch! > > %} > > > > /* Default code for all wrapped proxy classes in Python. > > > > > > Cheers, > > Daniel > 0: svn diff -r855533:855534 ^/subversion/branches/python-bindings-improvements/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c@855534
