cpython icon indicating copy to clipboard operation
cpython copied to clipboard

Published 20 hours ago • verified publisher icon python

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

Open yanjs opened this issue 2 years ago • 5 comments

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

  • Issue: gh-98248

yanjs avatar Oct 13 '22 22:10 yanjs

Most changes to Python require a NEWS entry.

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

bedevere-bot avatar Oct 13 '22 22:10 bedevere-bot

All commit authors signed the Contributor License Agreement.
CLA signed

cpython-cla-bot[bot] avatar Oct 13 '22 22:10 cpython-cla-bot[bot]

Reference to a related PR: https://github.com/python/cpython/pull/28178

yanjs avatar Oct 14 '22 02:10 yanjs

Completed. Any comments or suggestions would be greatly appreciated.

yanjs avatar Oct 14 '22 03:10 yanjs

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.

yanjs avatar Oct 14 '22 14:10 yanjs

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 avatar Oct 25 '22 20:10 yanjs

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 avatar Oct 26 '22 18:10 mdickinson

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.

bedevere-bot avatar Nov 27 '22 11:11 bedevere-bot

I have made the requested changes; please review again

yanjs avatar Nov 29 '22 02:11 yanjs

Thanks for making the requested changes!

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

bedevere-bot avatar Nov 29 '22 02:11 bedevere-bot

:robot: New build scheduled with the buildbot fleet by @mdickinson for commit 5b6fd702b2a6f4182ab1f648aba3ab4b20f82223 :robot:

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

bedevere-bot avatar Dec 04 '22 13:12 bedevere-bot

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 avatar Dec 04 '22 20:12 mdickinson

thanks

yanjs avatar Dec 05 '22 08:12 yanjs