Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added simple sys.getrefcount tests of current baseline #3030

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mgeplf
Copy link
Collaborator

@mgeplf mgeplf commented Aug 6, 2024

No description provided.


#todo: need to find hoc_built_in_symlist values

# not sure what the purpose of this is; return a dict of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is vital, it's how you discover (some of) the available "things" like mechanisms, or methods (along with some things that aren't supposed to be there).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's the last resort, normally I would expect dir() to be used, which relies on Py_tp_methods and things like Py_tp_getset.

Anyways, not important for this review, I will remove the not sure as it's not relevant.


def test_section():
section = h.Section(name='section')
assert sys.getrefcount(section) == 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. The following tedious proposition:

Suggested change
assert sys.getrefcount(section) == 2
base_refcount = 2
assert sys.getrefcount(section) == base_refcount

Here it's pointless. It does protect a little against there currently being 2 references (when I can only see 1).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the extra one is the call to sys.getrefcount, I'll make a comment.
I thought about having a base_refcount like variable, so that's 2 of us.
I'll make the change.


# {Py_tp_repr, (void*) pysec_repr_safe},
repr(section)
assert sys.getrefcount(section) == 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's where we gain something:

    assert sys.getrefcount(section) == base_refcount

which one would interpret as base_refcount + 0, i.e. it correctly returned to +0.

#static PyType_Slot nrnpy_AllSegOfSecIterType_slots[] = {
it = section.allseg()
assert sys.getrefcount(it) == 2
assert sys.getrefcount(section) == 3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert sys.getrefcount(section) == 3
assert sys.getrefcount(section) == base_refcount + 1

The +1, because we're holding one additional reference.

Copy link

✔️ cf1da10 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@mgeplf mgeplf force-pushed the add-python-refcount-tests branch from cf1da10 to 766bdff Compare August 6, 2024 11:59
Copy link

✔️ 766bdff -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ cf7ffa3 -> Azure artifacts URL

Copy link

sonarcloud bot commented Aug 7, 2024

Copy link

✔️ 416cc15 -> Azure artifacts URL

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.32%. Comparing base (bd5400b) to head (416cc15).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3030      +/-   ##
==========================================
+ Coverage   67.27%   67.32%   +0.05%     
==========================================
  Files         572      572              
  Lines      104928   104923       -5     
==========================================
+ Hits        70591    70643      +52     
+ Misses      34337    34280      -57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants