You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2017/12/23 05:11:05 UTC
Re: svn commit: r1819110 -
/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
troycurtisjr@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?
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?).
> 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.
> %}
>
> /* Default code for all wrapped proxy classes in Python.
>
>
Cheers,
Daniel
Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
Posted by Troy Curtis Jr <tr...@gmail.com>.
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:
> >
> > > troycurtisjr@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
>
Re: svn commit: r1819110 -
/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Troy Curtis Jr wrote on Wed, 27 Dec 2017 02:48 +0000:
> In taking another look at why hasattr() seeming fails in certain cases on
> SWIG generated new style classes under python2, I ran across this article,
> https://hynek.me/articles/hasattr/, which confirms that python 2's
> hasattr() can misbehave in the presence of properties (which the
> non-classic SWIG classes use). So this appears to be a known discrepancy.
> I did change the code to no hide any AttributeError's encountered in the
> assert_valid() method call itself.
If I'm reading the article correctly, it didn't actually explain why
hasattr() false negatived, but it did say that hasattr() is an
antipattern, which supports r1819110.
Fair enough :-)
Thanks,
Daniel
Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
Posted by Troy Curtis Jr <tr...@gmail.com>.
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:
> >
> > > troycurtisjr@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).
>
>
In taking another look at why hasattr() seeming fails in certain cases on
SWIG generated new style classes under python2, I ran across this article,
https://hynek.me/articles/hasattr/, which confirms that python 2's
hasattr() can misbehave in the presence of properties (which the
non-classic SWIG classes use). So this appears to be a known discrepancy.
I did change the code to no hide any AttributeError's encountered in the
assert_valid() method call itself.
Troy
Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
Posted by Troy Curtis Jr <tr...@gmail.com>.
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:
> >
> > > troycurtisjr@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
>
Re: svn commit: r1819110 -
/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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:
>
> > troycurtisjr@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@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
Re: svn commit: r1819110 -
/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
Posted by Branko Čibej <br...@apache.org>.
On 23.12.2017 16:27, Troy Curtis Jr wrote:
>
> On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <d.s@daniel.shahaf.name
> <ma...@daniel.shahaf.name>> wrote:
>
> 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.
It's not necessarily a no-op: depends on the type of 'x', if it's
smaller than an int, then 'x | 0' involves an implicit type conversion.
-- Brane
Re: svn commit: r1819110 -
/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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:
>
> > troycurtisjr@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@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
Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> troycurtisjr@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
Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> troycurtisjr@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