Refactor setattrbehavior calls

[frmdstryr]

Please double check my usage of valueptr. It used valueptr.release() before but since the args doesn't steal a reference I believe valueptr.get() is correct.

[frmdstryr]

I'm also somewhat confused with the original code... doesn't it all need updated to use

cppy::ptr ok(...)
if (!ok)
  return -1

??

[MatthieuDartiailh]

I agree that there is no reference stealing happening in the new version and that your usage of valueptr looks fine to me.

[MatthieuDartiailh]

I'm also somewhat confused with the original code... doesn't it all need updated to use

cppy::ptr ok(...)
if (!ok)
  return -1

??

Can you specify which line you are referring to ?

[frmdstryr]

Like here https://github.com/nucleic/atom/blob/main/atom/src/setattrbehavior.cpp#L321

I think that is leaking a reference to the returned value?

[MatthieuDartiailh]

I think you are correct. I need to look into stricter tests for references leaks !

[sccolbert]

Like here https://github.com/nucleic/atom/blob/main/atom/src/setattrbehavior.cpp#L321

I think that is leaking a reference to the returned value?

setattr is somewhat special in that it doesn't return a value. It either throws an exception (indicated by returning -1) or succeeds (indicated by returning 0).

[sccolbert]

Like here https://github.com/nucleic/atom/blob/main/atom/src/setattrbehavior.cpp#L321

I think that is leaking a reference to the returned value?

This is not leaking. cppy::ptr assumes you are supplying a new references to the constructor, and it decrefs that ownership on the destructor.

[sccolbert]

Wait, I'm wrong again. Dammit I'm out of practice.

Yes, you are correct. The PyObject_Call returns a new reference. In the context of setattr this should be an exception (0 or nullptr for failure) or (Py_None for success).

So, we do need to trap that in a cppy::ptr to be correct. Even though leaking refs to the Py_None singleton is not a huge deal.

[MatthieuDartiailh]

But the return value from call is a PyObject*. In general it should be None which is immortal so leaking a reference does not matter but to be perfectly correct we should still decrease it, no ?

[MatthieuDartiailh]

Then I made a not too bad mistake quite some time ago and we can now fix it. Good.

[frmdstryr]

It might be less error prone make the handler's return a PyObject* and do one check in Member::setattr?

Edit: Nevermind I guess that wouldn't always work.

[sccolbert]

It might be less error prone make the handler's return a PyObject* and do one check in Member::setattr?

Edit: Nevermind I guess that wouldn't always work.

While I wouldn't be opposed to that in theory, it would touch a lot of the other various setattr handlers, for little gain IMO. The setattr behavior really is unique within the Python C-api. My guess is that the core devs would change it if it wouldn't break the world.

[frmdstryr]

It looks like full_validate is not called on the propery_handler value?

[sccolbert]

It looks like full_validate is not called on the propery_handler value?

IIRC that is correct. IIRC there wasn't enough room in the Member struct to store a validate handler and a setattr handler. So the assumption was that the setattr function would handle everything, since it also has to handle the storage of the actual value.

[sccolbert]

It looks like full_validate is not called on the propery_handler value?

IIRC that is correct. IIRC there wasn't enough room in the Member struct to store a validate handler and a setattr handler. So the assumption was that the setattr function would handle everything, since it also has to handle the storage of the actual value.

Keep in mind that this library is well over 10 years old now and was heavily influenced by Enthought Traits. The Property member has always been a bit of a special case. Since it delegates the get/set behavior, it's very unclear who should own the actual value and the validation thereof.

[MatthieuDartiailh]

To be honest the Property member and its behavior are weird to me. I will let @sccolbert comment.

[sccolbert]

To be honest the Property member and its behavior are weird to me. I will let @sccolbert comment.

Yeah. If I were to write the library again today, Property would not be a part of it.

[MatthieuDartiailh]

Like you started the discussion on enaml maybe we could start a discussion on atom.

[sccolbert]

Like you started the discussion on enaml maybe we could start a discussion on atom.

Let's do it!!!!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.67%. Comparing base (7030a07) to head (bd7395f).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #227   +/-   ##
=======================================
  Coverage   97.67%   97.67%           
=======================================
  Files          24       24           
  Lines        1074     1074           
  Branches      162      162           
=======================================
  Hits         1049     1049           
  Misses         12       12           
  Partials       13       13