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

gh-109802: removed inaccessible code from complexobject.c #109642

Closed
wants to merge 56 commits into from
Closed

gh-109802: removed inaccessible code from complexobject.c #109642

wants to merge 56 commits into from

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Sep 21, 2023 •
edited
Loading

  • c_powu: L189 (c_powu() called with n < 100 and mask can't overflow)
  • complex_hash: L459, L462 (in current implementation of hashing, -1 is reserved)
  • complex_richcompare: L643, L669 (TO_COMPLEX calls are redundant)
  • complex_subtype_from_string: L892 (complex_subtype_from_string() helper called only on L949 with unicode object at v)
  • actual_complex_new: L960 (PyComplex_Check can't be true here), L960 (test added)
  • complex_new_impl: L1028 (test added)
  • complex_new_impl: L1034 (here r is not a complex subtype, hence above try_complex_special_method() call was unsuccessful)
  • complex_new_impl: L1068 (here r is a complex subtype, hence own_r==1)
  • complex_from_number: L1150 (test added)

Line numbers are wrt to complexobject.c at 8a284e189673582e2.

Everything (except for memory errors) should be covered by tests in test_complex.py and test_capi/test_complex.py.

@skirpichev
pythongh-109802: Increase test coverage for complexobject.c
facb60b 
* _Py_c_pow: L134 (this case goes to c_powi() due to L523), L139

// line numbers wrt to 54fbfa8
@skirpichev
* c_powu: L166
deff888
@skirpichev
* complex_hash: L411, L414, L423
8a7b344 
https://docs.python.org/3/library/stdtypes.html#hashing-of-numeric-types

Inaccessible cases transformed to asserts.
@skirpichev
* to_complex: L443, L449, L453-454
e383167
@skirpichev
* complex_sub: L474, L475
85fae1a
@skirpichev
* complex_mul: L485, L486
406c375
@skirpichev
* complex_div: L496, L497
aba6fb4
@skirpichev
* complex_pow: L512, L513, L522
204e2ec
@skirpichev
* complex_pos: L555-L559
b67e8d6
@skirpichev
* complex_richcompare: L595, L616, L621, L625
85f6994
@skirpichev
* complex_from_string_inner: L786, L804
ffd088a
@skirpichev
* complex_subtype_from_string: L860, L871-L874 (the function called i…
72b349d 
…n L926)
@skirpichev
* complex_new_impl: L944, L951, L958, L965, L985, L994
c25563c
@skirpichev
* complex_bool: L580
e1c247f
@skirpichev skirpichev changed the title Increase test coverage for complexobject.c gh-109802: Increase test coverage for complexobject.c Sep 24, 2023
@bedevere-app bedevere-app bot mentioned this pull request Sep 24, 2023
Closed
skirpichev added a commit to skirpichev/cpython that referenced this pull request Sep 27, 2023
@skirpichev
More tests for complex_add/sub/neg()
3808d5f 
Partially from python#109642
skirpichev added a commit to skirpichev/cpython that referenced this pull request Sep 27, 2023
@skirpichev
More tests for complex_mul()
b497e8f 
Partially from python#109642
eendebakpt
eendebakpt reviewed Nov 10, 2023
View reviewed changes
Objects/complexobject.c Outdated Show resolved Hide resolved
Objects/complexobject.c Outdated Show resolved Hide resolved
eendebakpt
eendebakpt approved these changes Nov 11, 2023
View reviewed changes
Copy link
Contributor

@eendebakpt eendebakpt left a comment

Choose a reason for hiding this comment

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

The PR increases test coverage in two ways:: adding tests and removing some code paths that cannot be reached. The added tests are quite safe to add. The changes to the C code are a bit hard to comprehend, but I checked them and seem valid to me. Also they are guarded by asserts.

Only one comment on a docstring, but otherwise looks good.

Objects/complexobject.c Outdated Show resolved Hide resolved
Objects/complexobject.c Show resolved Hide resolved
Objects/complexobject.c Show resolved Hide resolved
@skirpichev @eendebakpt
Apply suggestions from code review
64f2174 
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
@skirpichev skirpichev marked this pull request as ready for review May 31, 2024 10:12
@mdickinson
Copy link
Member

Please could one of the core developers watching this PR remove the request for my review? Thank you!

@serhiy-storchaka serhiy-storchaka removed the request for review from mdickinson August 13, 2024 17:14
@skirpichev skirpichev requested a review from picnixz October 6, 2024 03:48
@skirpichev
Copy link
Member Author

@picnixz, this is quite old PR with a bunch of nitpicks. I would appreciate if you take a look.

picnixz
picnixz reviewed Oct 6, 2024
View reviewed changes
Objects/complexobject.c Outdated Show resolved Hide resolved
Objects/complexobject.c Outdated Show resolved Hide resolved
Objects/complexobject.c Outdated Show resolved Hide resolved
Objects/complexobject.c Outdated Show resolved Hide resolved
Objects/complexobject.c Show resolved Hide resolved
@skirpichev @picnixz
Apply suggestions from code review
d9f2c67 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@skirpichev skirpichev requested a review from picnixz October 6, 2024 10:38
skirpichev
skirpichev commented Oct 6, 2024
View reviewed changes
Objects/complexobject.c Outdated Show resolved Hide resolved
picnixz
picnixz reviewed Oct 6, 2024
View reviewed changes
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Final nitpicks that I missed. Otherwise I think it's ok? (I'm not familiar enough with this part of the code but it looks good to me).

Objects/complexobject.c Outdated Show resolved Hide resolved
Objects/complexobject.c Show resolved Hide resolved
skirpichev
skirpichev commented Oct 6, 2024
View reviewed changes
Objects/complexobject.c Outdated Show resolved Hide resolved
@skirpichev skirpichev requested a review from picnixz October 7, 2024 01:52
picnixz
picnixz approved these changes Oct 7, 2024
View reviewed changes
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Just a question otherwise LGTM.

Objects/complexobject.c Show resolved Hide resolved
@Coffigny94

This comment was marked as off-topic.

Sign in to view
@skirpichev
Copy link
Member Author

@serhiy-storchaka, I suspect that @mdickinson was -1 on this pr; thus, it should be closed together with the issue.

Let me know if it this make sense for you in some form at least partially (e.g. removing useless TO_COMPLEX or useless PyUnicode_Check()). If so, I'll quickly revert the rest.

@serhiy-storchaka
Copy link
Member

If @mdickinson was -1, I am -1 too. This issue already taken up too much of your and my time. Too much for any potential benefits.

@skirpichev skirpichev closed this Oct 14, 2024
@skirpichev skirpichev deleted the complex-cov branch October 14, 2024 16:51
@skirpichev
Copy link
Member Author

If @mdickinson was -1

That's only my guess.

Anyway, thank you for your time. @picnixz and @eendebakpt, thank for reviews.

@skirpichev skirpichev mentioned this pull request Oct 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@picnixz picnixz picnixz approved these changes

@mdickinson mdickinson mdickinson requested changes

@eendebakpt eendebakpt eendebakpt approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees
No one assigned
Labels
awaiting review skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@skirpichev @serhiy-storchaka @mdickinson @Coffigny94 @eendebakpt @picnixz @AlexWaygood