GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-109802: removed inaccessible code from complexobject.c by skirpichev · Pull Request #109642 · python/cpython

Conversation

@skirpichev

@skirpichev skirpichev commented Sep 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added the awaiting review label Sep 21, 2023
skirpichev added 14 commits September 24, 2023 13:06
@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 force-pushed the complex-cov branch from d270aa6 to e1c247f Compare September 24, 2023 10:06
@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
More tests for complexobject.c #109802
Closed
@skirpichev
Merge branch 'main' into complex-cov
0a8679f
@AlexWaygood AlexWaygood added the skip news label Sep 24, 2023
@AlexWaygood
AlexWaygood requested a review from mdickinson September 24, 2023 14:29
@skirpichev
Merge branch 'main' into complex-cov
f96c0ce
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
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev
Merge branch 'main' into complex-cov
fbd035d
eendebakpt
eendebakpt approved these changes Nov 11, 2023
View reviewed changes

@eendebakpt eendebakpt left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Nov 11, 2023
@skirpichev @eendebakpt
Apply suggestions from code review
64f2174 
Co-authored-by: Pieter Eendebak <[email protected]>
@skirpichev
skirpichev marked this pull request as ready for review May 31, 2024 10:12
@bedevere-app bedevere-app Bot added the awaiting review label May 31, 2024
@skirpichev
Merge branch 'main' into complex-cov
1781c95
eendebakpt
eendebakpt approved these changes Jul 24, 2024
View reviewed changes
@mdickinson

mdickinson commented Aug 13, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

skirpichev commented Oct 6, 2024

Copy link
Copy Markdown
Member Author

@picnixz, this is quite old PR with a bunch of nitpicks. I would appreciate if you take a look.
picnixz reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz reviewed Oct 6, 2024
View reviewed changes
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
skirpichev
skirpichev commented Oct 6, 2024
View reviewed changes
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev @picnixz
Apply suggestions from code review
d9f2c67 
Co-authored-by: Bénédikt Tran <[email protected]>
@skirpichev
skirpichev requested a review from picnixz October 6, 2024 10:38
skirpichev
skirpichev commented Oct 6, 2024
View reviewed changes
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev
Update Objects/complexobject.c
6cad37a
picnixz
picnixz reviewed Oct 6, 2024
View reviewed changes

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
skirpichev
skirpichev commented Oct 6, 2024
View reviewed changes
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev
Apply suggestions from code review
f1f3d97
@skirpichev
skirpichev requested a review from picnixz October 7, 2024 01:52
picnixz
picnixz approved these changes Oct 7, 2024
View reviewed changes

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/complexobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Coffigny94

This comment was marked as off-topic.

Sign in to view
@skirpichev
skirpichev requested a review from serhiy-storchaka October 8, 2024 07:16
@skirpichev

skirpichev commented Oct 8, 2024

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
skirpichev added 2 commits October 13, 2024 12:51
@skirpichev
Merge branch 'master' into complex-cov
7ef464d
@skirpichev
Add test for complex_from_number
65c30f1
@serhiy-storchaka

serhiy-storchaka commented Oct 14, 2024

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev skirpichev closed this Oct 14, 2024
@skirpichev
skirpichev deleted the complex-cov branch October 14, 2024 16:51
@skirpichev

skirpichev commented Oct 14, 2024

Copy link
Copy Markdown
Member Author

If @mdickinson was -1

That's only my guess.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev skirpichev mentioned this pull request Oct 25, 2024
gh-102837: Increase test coverage for the math module #110000
Closed
@skirpichev skirpichev mentioned this pull request Jan 14, 2026
Remove unreachable else-block in complex_subtype_from_string in Objects/complexobject.c #143830
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
@eendebakpt eendebakpt eendebakpt approved these changes
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka
+1 more reviewer
@mdickinson mdickinson mdickinson requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@skirpichev @serhiy-storchaka @mdickinson @Coffigny94 @eendebakpt @picnixz @AlexWaygood
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.