Skip to content

bpo-40455: Remove gcc10 warning about x_digits#19852

Merged
corona10 merged 3 commits into
python:masterfrom
corona10:bpo-40455
May 4, 2020
Merged

bpo-40455: Remove gcc10 warning about x_digits#19852
corona10 merged 3 commits into
python:masterfrom
corona10:bpo-40455

Conversation

@corona10

@corona10 corona10 commented May 2, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@corona10 corona10 requested a review from mdickinson May 2, 2020 02:50
mdickinson
mdickinson suggested changes May 2, 2020
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The approach looks good to me, but there's a logic error to fix. I'm a little bit disturbed that we didn't get more test failures here.

Comment thread Objects/longobject.c Outdated
@bedevere-bot

bedevere-bot commented May 2, 2020

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@corona10

corona10 commented May 2, 2020

Copy link
Copy Markdown
Member Author

Thank you for the review and for catching the missed thing :)
I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented May 2, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@mdickinson: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from mdickinson May 2, 2020 13:29
mdickinson
mdickinson approved these changes May 2, 2020
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thank you.

@corona10

corona10 commented May 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

@vstinner Since I am still in the mentoring period, I will merge this after your confirm :)

@corona10 corona10 self-assigned this May 2, 2020
shihai1991
shihai1991 approved these changes May 3, 2020
View reviewed changes

@shihai1991 shihai1991 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

vstinner
vstinner approved these changes May 4, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

My main worry is that this change will initialize twice a few digits and so it would make the code less efficient. But @mdickinson approved the change, so I'm fine with it :-)

The change is correct. Initializing digits to zero cannot be wrong :-)

@corona10 corona10 merged commit b88cd58 into python:master May 4, 2020
@bedevere-bot

bedevere-bot commented May 4, 2020

Copy link
Copy Markdown

@corona10: Please replace # with GH- in the commit message next time. Thanks!

@corona10 corona10 deleted the bpo-40455 branch May 4, 2020 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@shihai1991 shihai1991 shihai1991 approved these changes

+1 more reviewer

@mdickinson mdickinson mdickinson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@corona10 corona10

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@corona10 @bedevere-bot @vstinner @mdickinson @shihai1991 @the-knights-who-say-ni