Skip to content

bpo-39648:[WIP]updated math.gcd to accept 'n' arguments.#18590

Closed
ananthan-123 wants to merge 17 commits intopython:masterfrom
ananthan-123:gcdnew
Closed

bpo-39648:[WIP]updated math.gcd to accept 'n' arguments.#18590
ananthan-123 wants to merge 17 commits intopython:masterfrom
ananthan-123:gcdnew

Conversation

@ananthan-123
Copy link
Copy Markdown
Contributor

@ananthan-123 ananthan-123 commented Feb 21, 2020

@ananthan-123 ananthan-123 changed the title bpo-39648:updated math.gcd to accept 'n' arguments. bpo-39648:[WIP]updated math.gcd to accept 'n' arguments. Feb 21, 2020
@ananthan-123
Copy link
Copy Markdown
Contributor Author

I made clinic and now it is showing checksum error.
What to do?

@mdickinson
Copy link
Copy Markdown
Member

mdickinson commented Feb 21, 2020

What error are you getting? make clinic runs fine for me on commit 660448e, and has no effect on mathmodule.c.h, so it looks as though everything is up to date here.

@mdickinson
Copy link
Copy Markdown
Member

mdickinson commented Feb 21, 2020

Ah, I think I understand: after you merge the upstream master in, you'll get conflicts in mathmodule.c.h. You can either restore mathmodule.c.h to either the local version or the upstream version (check out git checkout --ours and git checkout --theirs), or perhaps more simply, I believe it should always be safe to simply delete mathmodule.c.h entirely and then regenerate it using make clinic.

@ananthan-123
Copy link
Copy Markdown
Contributor Author

I think the test failure is due to some other problem!!

@mdickinson
Copy link
Copy Markdown
Member

I think the test failure is due to some other problem!!

Yes. You need to initialise g to be the integer zero (it's currently initialised to the NULL pointer, and then that NULL pointer gets passed into the first call to _PyLong_GCD). After that, there are some reference leaks to sort out.

Copy link
Copy Markdown
Member

@sweeneyde sweeneyde left a comment

Choose a reason for hiding this comment

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

I think the main bug is that there is a difference between the C value 0 (aka a null pointer) and the Python int 0.

Comment thread Modules/mathmodule.c Outdated
Comment thread Modules/mathmodule.c
Comment thread Lib/test/test_math.py
Comment thread Lib/test/test_math.py
Comment thread Doc/library/math.rst Outdated
Comment thread Modules/mathmodule.c Outdated
Comment thread Lib/test/test_math.py
Comment thread Lib/test/test_math.py Outdated
Comment thread Lib/test/test_math.py Outdated
Comment thread Modules/mathmodule.c
@ananthan-123
Copy link
Copy Markdown
Contributor Author

Updated the code.
Do we have to decref 'item'.

Comment thread Modules/mathmodule.c
@mdickinson
Copy link
Copy Markdown
Member

Do we have to decref 'item'.

No: item is effectively a borrowed reference; no need to decref. Having said that, you don't really need a separate item at all. You could replace:

item = args[i];
in = PyNumber_Index(item);

with simply

in = PyNumber_Index(args[i]);

@ananthan-123
Copy link
Copy Markdown
Contributor Author

Thanks for the review.Made suitable changes.

@ananthan-123
Copy link
Copy Markdown
Contributor Author

Now the problem is with clinic(don't know if any other problem is there).Don't we have to make clinic.
Do we have to regain previous clinic file.

@mdickinson
Copy link
Copy Markdown
Member

Closing, since #18604 was merged.

@mdickinson mdickinson closed this Feb 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants