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.

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

gh-98248: Normalizing the error messages in function struct.pack #98252

Merged
merged 21 commits into from
Dec 4, 2022
Merged

gh-98248: Normalizing the error messages in function struct.pack #98252

merged 21 commits into from
Dec 4, 2022

Conversation

yanjs
Copy link
Contributor

@yanjs yanjs commented Oct 13, 2022

Provide informative error messages in function struct.pack when its integral arguments are not in range.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Oct 13, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@yanjs yanjs changed the title gh-98248: fix wrong error messages in struct.pack gh-98248: Normalizing the error messages in function struct.pack Oct 13, 2022
@yanjs
Copy link
Contributor Author

yanjs commented Oct 14, 2022

Reference to a related PR: #28178

@yanjs
Copy link
Contributor Author

yanjs commented Oct 14, 2022

Completed. Any comments or suggestions would be greatly appreciated.

@kumaraditya303 kumaraditya303 added type-feature A feature request or enhancement extension-modules C modules in the Modules dir labels Oct 14, 2022
@yanjs
Copy link
Contributor Author

yanjs commented Oct 14, 2022

Should I change the definition of macro RANGE_ERROR and function _range_error in Modules/_struct.c? Some arguments of RANGE_ERROR are not used. _range_error cannot handle long long and unsigned long long. I didn't change them since they are used by some other correct functions.

Modules/_struct.c Outdated Show resolved Hide resolved
@yanjs
Copy link
Contributor Author

yanjs commented Oct 25, 2022

Thank you for your reviews, @kumaraditya303 @mdickinson. I have one more question. Should I remove parameters x and mask from macro RANGE_ERROR in the previous commit 61c016b Modules/_struct.c line 278?

Reasons not to make this change:

  1. Some other working lines of code are modified.
  2. In future, more detailed error messages can use the x.

Reasons to make this change:

  1. x and mask are not used in the macro body.
  2. In 61c016b, Modules/_struct.c line 672, x may be undefined, which is not good. It will not cause problems in runtime since this x will be erased by the macro.

I will make this change in the next commit 856a9be. If you feel this change is unnecessary, you can revert this commit.
I think the error in the automatic check is not related.

@yanjs yanjs requested a review from mdickinson October 25, 2022 21:41
@yanjs yanjs requested a review from mdickinson October 25, 2022 21:41
@mdickinson
Copy link
Member

Thanks for the updates. It might be a few days before I have time to re-review; feel free to ping if you don't hear anything by mid-November.

Should I remove parameters x and mask from macro RANGE_ERROR

Sounds fine to me in principle.

@mdickinson mdickinson self-assigned this Oct 26, 2022
Modules/_struct.c Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@yanjs
Copy link
Contributor Author

yanjs commented Nov 29, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

Thank you for the updates! This LGTM, and works as advertised in manual testing.

I'll run this through the buildbots, and then if all's okay I'll merge.

@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 4, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit 5b6fd70 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 4, 2022
@mdickinson
Copy link
Member

mdickinson commented Dec 4, 2022

Two buildbot failures, neither of which looks relevant to the changes in this PR (one failure in test_cmd_line_script; one in test_capi). One buildbot has yet to report, but I think at this point it's safe to merge.

@yanjs Thank you for the contribution and for your patience!

@mdickinson mdickinson merged commit 854a878 into python:main Dec 4, 2022
@yanjs
Copy link
Contributor Author

yanjs commented Dec 5, 2022

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants