gh-109802: removed inaccessible code from complexobject.c by skirpichev · Pull Request #109642 · python/cpython

skirpichev · 2023-09-21T08:13:53Z

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.

Issue: More tests for complexobject.c #109802

* _Py_c_pow: L134 (this case goes to c_powi() due to L523), L139 // line numbers wrt to 54fbfa8

https://docs.python.org/3/library/stdtypes.html#hashing-of-numeric-types Inaccessible cases transformed to asserts.

…n L926)

Partially from python#109642

eendebakpt

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.

Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>

mdickinson · 2024-08-13T17:06:24Z

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

skirpichev · 2024-10-06T03:49:47Z

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

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

picnixz

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).

picnixz

Just a question otherwise LGTM.

skirpichev · 2024-10-08T07:21:17Z

@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 · 2024-10-14T16:43:09Z

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 · 2024-10-14T17:03:29Z

If @mdickinson was -1

That's only my guess.

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

bedevere-appBot added the awaiting review label Sep 21, 2023

skirpichev added 14 commits September 24, 2023 13:06

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

* c_powu: L166
deff888

* complex_hash: L411, L414, L423
8a7b344
https://docs.python.org/3/library/stdtypes.html#hashing-of-numeric-types Inaccessible cases transformed to asserts.

* to_complex: L443, L449, L453-454
e383167

* complex_sub: L474, L475
85fae1a

* complex_mul: L485, L486
406c375

* complex_div: L496, L497
aba6fb4

* complex_pow: L512, L513, L522
204e2ec

* complex_pos: L555-L559
b67e8d6

* complex_richcompare: L595, L616, L621, L625
85f6994

* complex_from_string_inner: L786, L804
ffd088a

* complex_subtype_from_string: L860, L871-L874 (the function called i…
72b349d
…n L926)

* complex_new_impl: L944, L951, L958, L965, L985, L994
c25563c

* complex_bool: L580
e1c247f

skirpichev force-pushed the complex-cov branch from d270aa6 to e1c247fCompare September 24, 2023 10:06

skirpichev changed the title ~~Increase test coverage for complexobject.c~~gh-109802: Increase test coverage for complexobject.cSep 24, 2023

bedevere-appBot mentioned this pull request Sep 24, 2023
More tests for complexobject.c #109802
Closed

Merge branch 'main' into complex-cov
0a8679f

AlexWaygood added the skip news label Sep 24, 2023

AlexWaygood requested a review from mdickinson September 24, 2023 14:29

Merge branch 'main' into complex-cov
f96c0ce

skirpichev added a commit to skirpichev/cpython that referenced this pull request Sep 27, 2023
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
More tests for complex_mul()
b497e8f
Partially from python#109642

eendebakpt reviewed Nov 10, 2023
View reviewed changes

Comment threadObjects/complexobject.c Outdated
Comment threadObjects/complexobject.c Outdated

Merge branch 'main' into complex-cov
fbd035d

eendebakpt approved these changes Nov 11, 2023
View reviewed changes

Comment threadObjects/complexobject.c Outdated
Comment threadObjects/complexobject.c
Comment threadObjects/complexobject.c

bedevere-appBot added awaiting core review and removed awaiting review labels Nov 11, 2023

Apply suggestions from code review
64f2174
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>

skirpichev marked this pull request as ready for review May 31, 2024 10:12

bedevere-appBot added the awaiting review label May 31, 2024

Merge branch 'main' into complex-cov
1781c95

eendebakpt approved these changes Jul 24, 2024
View reviewed changes

serhiy-storchaka removed the request for review from mdickinson August 13, 2024 17:14

skirpichev requested a review from picnixz October 6, 2024 03:48

picnixz reviewed Oct 6, 2024
View reviewed changes

Comment threadObjects/complexobject.c Outdated
Comment threadObjects/complexobject.c Outdated
Comment threadObjects/complexobject.c Outdated
Comment threadObjects/complexobject.c Outdated
Comment threadObjects/complexobject.c

skirpichev commented Oct 6, 2024
View reviewed changes

Comment threadObjects/complexobject.c

Apply suggestions from code review
d9f2c67
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

skirpichev requested a review from picnixz October 6, 2024 10:38

skirpichev commented Oct 6, 2024
View reviewed changes

Comment threadObjects/complexobject.c Outdated

Update Objects/complexobject.c
6cad37a

picnixz reviewed Oct 6, 2024
View reviewed changes

Comment threadObjects/complexobject.c Outdated
Comment threadObjects/complexobject.c

skirpichev commented Oct 6, 2024
View reviewed changes

Comment threadObjects/complexobject.c Outdated

Apply suggestions from code review
f1f3d97

skirpichev requested a review from picnixz October 7, 2024 01:52

picnixz approved these changes Oct 7, 2024
View reviewed changes

Comment threadObjects/complexobject.c

This comment was marked as off-topic.
Sign in to view

skirpichev requested a review from serhiy-storchaka October 8, 2024 07:16

skirpichev added 2 commits October 13, 2024 12:51

Merge branch 'master' into complex-cov
7ef464d

Add test for complex_from_number
65c30f1

skirpichev closed this Oct 14, 2024

skirpichev deleted the complex-cov branch October 14, 2024 16:51

skirpichev mentioned this pull request Oct 25, 2024
gh-102837: Increase test coverage for the math module #110000
Closed

skirpichev mentioned this pull request Jan 14, 2026
Remove unreachable else-block in complex_subtype_from_string in Objects/complexobject.c #143830
Closed

Uh oh!

Uh oh!

Conversation

skirpichev commented Sep 21, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eendebakpt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdickinson commented Aug 13, 2024

Uh oh!

skirpichev commented Oct 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as off-topic.

skirpichev commented Oct 8, 2024

Uh oh!

serhiy-storchaka commented Oct 14, 2024

Uh oh!

skirpichev commented Oct 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

skirpichev commented Sep 21, 2023•
edited
Loading