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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Lib/test/clinic.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

if (a == -1 && PyErr_Occurred()) {
goto exit;
}
skip_optional:
Expand All @@ -1281,7 +1281,7 @@ exit:

static PyObject *
test_long_long_converter_impl(PyObject *module, long long a)
/*[clinic end generated code: output=3e8083f3aee4f18a input=d5fc81577ff4dd02]*/
/*[clinic end generated code: output=7143b585d7e433e8 input=d5fc81577ff4dd02]*/


/*[clinic input]
Expand Down
4 changes: 2 additions & 2 deletions Modules/_testcapimodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4704,8 +4704,8 @@ dict_get_version(PyObject *self, PyObject *args)

version = dict->ma_version_tag;

Py_BUILD_ASSERT(sizeof(unsigned PY_LONG_LONG) >= sizeof(version));
return PyLong_FromUnsignedLongLong((unsigned PY_LONG_LONG)version);
Py_BUILD_ASSERT(sizeof(unsigned long long) >= sizeof(version));
return PyLong_FromUnsignedLongLong((unsigned long long)version);
}


Expand Down
2 changes: 1 addition & 1 deletion PC/pyconfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ Py_NO_ENABLE_SHARED to find out. Also support MS_NO_COREDLL for b/w compat */
# define SIZEOF_HKEY 8
# define SIZEOF_SIZE_T 8
/* configure.ac defines HAVE_LARGEFILE_SUPPORT iff
sizeof(off_t) > sizeof(long), and sizeof(PY_LONG_LONG) >= sizeof(off_t).
sizeof(off_t) > sizeof(long), and sizeof(long long) >= sizeof(off_t).
On Win64 the second condition is not true, but if fpos_t replaces off_t
then this is true. The uses of HAVE_LARGEFILE_SUPPORT imply that Win64
should define this. */
Expand Down
2 changes: 1 addition & 1 deletion Tools/clinic/clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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? ;)

goto exit;
}}}}
""".format(argname=argname, paramname=self.name)
Expand Down