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.

Sign up for GitHub

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

Jump to bottom

bpo-12414: Update code_sizeof() to take in account co_extras #1168

Merged
merged 1 commit into from
Apr 20, 2017
Merged

bpo-12414: Update code_sizeof() to take in account co_extras #1168

merged 1 commit into from
Apr 20, 2017

Conversation

corona10
Copy link
Member

bpo-12414: code_sizeof() update to take in account co_extras

@mention-bot
Copy link

@corona10, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @serhiy-storchaka and @benjaminp to be potential reviewers.

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@corona10 corona10 force-pushed the fix-issue-12414 branch 2 times, most recently from 75d8644 to c2aad6e Compare April 18, 2017 08:50
@corona10 corona10 changed the title [WIP] bpo-12414: code_sizeof() update to take in account co_extras [WIP] bpo-12414: Update code_sizeof() to take in account co_extras Apr 18, 2017
serhiy-storchaka
serhiy-storchaka reviewed Apr 18, 2017
View reviewed changes
Objects/codeobject.c Outdated
if (co->co_cell2arg != NULL && co->co_cellvars != NULL)
res += PyTuple_GET_SIZE(co->co_cellvars) * sizeof(Py_ssize_t);

if (co_extra != NULL)
res += co_extra->ce_size * sizeof(co_extra->ce_extras[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Is this indentation correct?

Copy link
Member Author

@corona10 corona10 Apr 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@serhiy-storchaka
Oh! I will fix this indent!
Is there any tool for auto-formatting to solve this kind of indentation problem?
e.g make format

Copy link
Member

Choose a reason for hiding this comment

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

No tools for auto-formatting are used with CPython sources. You should follow PEP 7 and good sense.

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error cherry-pick for 3.6 and removed cherry-pick for 3.6 labels Apr 18, 2017
@corona10 corona10 changed the title [WIP] bpo-12414: Update code_sizeof() to take in account co_extras bpo-12414: Update code_sizeof() to take in account co_extras Apr 18, 2017
@corona10
Copy link
Member Author

@serhiy-storchaka Done! Thanks for the tip. PTAL

@corona10
Copy link
Member Author

FYI, I sent to @betswaliszewski about the CLA issue that I didn't create my account on bugs.python.org at that time. I created it today. And this issue will be solved.

@corona10
Copy link
Member Author

(CLA signed: http://bugs.python.org/user25975)

@corona10
Copy link
Member Author

@serhiy-storchaka Ready for review!

@corona10 corona10 mentioned this pull request Apr 20, 2017
Merged
serhiy-storchaka
serhiy-storchaka reviewed Apr 20, 2017
View reviewed changes
Objects/codeobject.c Outdated
@@ -452,10 +452,15 @@ static PyObject *
code_sizeof(PyCodeObject *co, void *unused)
{
Py_ssize_t res;

res = _PyObject_SIZE(Py_TYPE(co));
Copy link
Member

Choose a reason for hiding this comment

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

If remove an empty line between declaration and assignment it is worth to merge them in one declaration with initialization.

@serhiy-storchaka
Copy link
Member

Add an entry in Misc/NEWS. Include "Patch by Your Name."

And I think the test should be updated. We have no way to create co_extra, but if it is created automatically by the interpreter the test will fail. It should be weaked, use >= rather than strong ==.

@corona10
Copy link
Member Author

@serhiy-storchaka
I update codes except for test codes.

Question:
I understood your intent about unit test.
You mean tests should go this way.

assertGreater(sys.getsizeof(code_obj), sizeofstruct)

I can not find current tests. It would be I am not familiar with the CPython codebase
Is there any sys.getsizeof() test exists on test_code.py?

@louisom
Copy link
Contributor

louisom commented Apr 20, 2017

@corona10 You can found it at test_descr.py, test_dict.py, test_mmap.py, test_bytes.py, for example.

@serhiy-storchaka
Copy link
Member

sys.getsizeof() tests for basic types are located in test_sys.py.

You can just add some large value to the result in code_sizeof(), run all tests and see what tests are failed.

serhiy-storchaka
serhiy-storchaka reviewed Apr 20, 2017
View reviewed changes
Misc/NEWS Outdated
@@ -10,6 +10,10 @@ What's New in Python 3.7.0 alpha 1?
Core and Builtins
-----------------

- bpo-12414: sys.getsizeof() on a code object now returns the sizes
which includes the code struct and sizes of objects which it references
patch by Dong-hee Na
Copy link
Member

@serhiy-storchaka serhiy-storchaka Apr 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Add periods after the sentences. Capitalize the first letter of "Patch".

@corona10 corona10 force-pushed the fix-issue-12414 branch 2 times, most recently from 4ec8b79 to ecc22aa Compare April 20, 2017 06:48
@corona10
Copy link
Member Author

@serhiy-storchaka Done! PTAL

serhiy-storchaka
serhiy-storchaka reviewed Apr 20, 2017
View reviewed changes
Misc/ACKS Outdated
@@ -1738,3 +1738,4 @@ evilzero
Dhushyanth Ramasamy
Subhendu Ghosh
Sanjay Sundaresan
Dong-hee Na
Copy link
Member

Choose a reason for hiding this comment

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

Names are ordered in rough alphabetical order of surnames. Your name should be placed together with the names starting with D or N (I don't know what is your given name and what is your surname).

@corona10
Copy link
Member Author

@serhiy-storchaka Thank you for your response. I updated the ACK file.

@serhiy-storchaka serhiy-storchaka merged commit b4dc6af into python:master Apr 20, 2017
@serhiy-storchaka
Copy link
Member

Thank you for your contribution Dong-hee!

Now you can cherry-pick b4dc6af to 3.6.

corona10 added a commit to corona10/cpython that referenced this pull request Apr 20, 2017
@corona10
[3.6] bpo-12414: Update code_sizeof() to take in account co_extra mem…
c2ddfdf 
…ory. (python#1168)
serhiy-storchaka pushed a commit that referenced this pull request Apr 20, 2017
@corona10 @serhiy-storchaka
[3.6] bpo-12414: Update code_sizeof() to take in account co_extra mem…
df5df13 
…ory. (#1168) (#1198)
@corona10 corona10 deleted the fix-issue-12414 branch April 24, 2017 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@corona10 @mention-bot @the-knights-who-say-ni @serhiy-storchaka @louisom @Mariatta