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.

Sign up for GitHub

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

Jump to bottom

bpo-27961: Replace PY_LONG_LONG with long long #15386

Merged
merged 1 commit into from
Oct 21, 2019
Merged

bpo-27961: Replace PY_LONG_LONG with long long #15386

merged 1 commit into from
Oct 21, 2019

Conversation

sir-sigurd
Copy link
Contributor

@sir-sigurd sir-sigurd commented Aug 22, 2019 •
edited by bedevere-bot
Loading

@sir-sigurd sir-sigurd requested a review from a team as a code owner August 22, 2019 11:21
sir-sigurd
sir-sigurd commented Aug 22, 2019
View reviewed changes
Tools/clinic/clinic.py
@@ -3005,7 +3005,7 @@ def parse_arg(self, argname, argnum):
goto exit;
}}}}
{paramname} = PyLong_AsLongLong({argname});
if ({paramname} == (PY_LONG_LONG)-1 && PyErr_Occurred()) {{{{
if ({paramname} == -1 && PyErr_Occurred()) {{{{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to remove this cast as it's unneeded.

Copy link
Contributor

@aeros aeros Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I decided to remove this cast as it's unneeded.

Any idea as to why this cast was previously needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@serhiy-storchaka you committed this code in 32d96a2. Does it make sense to you to remove this cast?

Copy link
Member

Choose a reason for hiding this comment

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

It was copied from Python/getargs.c.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep the explicit cast, since long long is an uncommon type and I prefer to avoid bad surprises on some platforms.

Copy link
Contributor Author

@sir-sigurd sir-sigurd Aug 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@vstinner I don't really see a problem here, because implicit conversion of integers of the same signedness is well defined and always lossless. If it's so important it can be rewritten as:

if ({paramname} == -1LL && PyErr_Occurred()) {{{{

In that case there are no casts at all.

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really see a problem here, because implicit conversion of integers of the same signedness is well defined and always lossless

@vstinner @serhiy-storchaka So is this okay universally across the C-API or is it different in other areas? Of course it wouldn't be worth a PR on it's own since it's more of a conventional/styling decision, I just want to know in case I see a similar implicit integer conversion (signed -> signed or unsigned -> unsigned) without the cast in another PR or if the issue comes up again.

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea. The C language remains partially a mystery to me.

Copy link
Contributor

@aeros aeros Oct 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@vstinner

I have no idea. The C language remains partially a mystery to me.

If the C language is partially a mystery even to you, what hope do the rest of us mere mortals have? ;)

epicfaace
epicfaace reviewed Aug 23, 2019
View reviewed changes
Copy link
Contributor

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Do you know why the build is failing on azure pipelines?

@sir-sigurd
Copy link
Contributor Author

Not sure why it's failed, but seems OK now.

@sir-sigurd sir-sigurd mentioned this pull request Aug 27, 2019
Merged
@aeros aeros added type-feature A feature request or enhancement skip news labels Aug 27, 2019
vstinner
vstinner reviewed Aug 27, 2019
View reviewed changes
Lib/test/clinic.test
@@ -1269,7 +1269,7 @@ test_long_long_converter(PyObject *module, PyObject *const *args, Py_ssize_t nar
goto exit;
}
a = PyLong_AsLongLong(args[0]);
if (a == (PY_LONG_LONG)-1 && PyErr_Occurred()) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep the explicit cast.

@serhiy-storchaka serhiy-storchaka merged commit a9ed91e into python:master Oct 21, 2019
@sir-sigurd sir-sigurd deleted the py-long-long branch October 21, 2019 06:56
@vstinner
Copy link
Member

Thanks @sir-sigurd for the cleanup :-)

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@sir-sigurd
bpo-27961: Replace PY_LONG_LONG with long long. (pythonGH-15386)
19034f7
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@sir-sigurd @shihai1991
bpo-27961: Replace PY_LONG_LONG with long long. (pythonGH-15386)
a512dec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@aeros aeros aeros left review comments

@epicfaace epicfaace epicfaace approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
skip news type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sir-sigurd @vstinner @epicfaace @serhiy-storchaka @aeros @the-knights-who-say-ni @bedevere-bot