revlog: replace PyInt_AS_LONG with a more portable helper function
ClosedPublic

Authored by durin42 on Tue, Nov 6, 11:44 AM.

Details

Summary

PyInt_AS_LONG disappears on Python, and our previous #define was
producing some problems on Python 3. Let's give up and make an inline
helper function that makes this more sane.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
durin42 created this revision.Tue, Nov 6, 11:44 AM
yuja added a subscriber: yuja.Wed, Nov 7, 7:35 AM

+/* Convert a PyInt or PyLong to a long. Returns false if there is an
+ error, in which case an exception will already have been set. */
+static inline bool pylong_to_long(PyObject *pylong, long *out)

Nit: I prefer 0/-1 return value instead of bool since that's the convention
of CPython API.

+ *out = PyLong_AsLong(pylong);
+ return PyErr_Occurred() == NULL;

Perhaps, checking *out != -1 can be a fast path, though I don't know how
much we care about the performance here.

static int rustla_contains(rustlazyancestorsObject *self, PyObject *rev)
{
  • if (!(PyInt_Check(rev))) { + long lrev; + if (!pylong_to_long(rev, &lrev)) { return 0;

Needs PyErr_Clear() since non_int in self` isn't an error.

durin42 updated this revision to Diff 12469.Wed, Nov 7, 1:42 PM
In D5235#78172, @yuja wrote:

+/* Convert a PyInt or PyLong to a long. Returns false if there is an
+ error, in which case an exception will already have been set. */
+static inline bool pylong_to_long(PyObject *pylong, long *out)

Nit: I prefer 0/-1 return value instead of bool since that's the convention
of CPython API.

I got lazy, but we can change it if we like.

+ *out = PyLong_AsLong(pylong);
+ return PyErr_Occurred() == NULL;

Perhaps, checking *out != -1 can be a fast path, though I don't know how
much we care about the performance here.

I don't either, but it was easy to add the if statement.

static int rustla_contains(rustlazyancestorsObject *self, PyObject *rev)
{
  • if (!(PyInt_Check(rev))) { + long lrev; + if (!pylong_to_long(rev, &lrev)) { return 0;

Needs PyErr_Clear() since non_int in self` isn't an error.

Good catch, fixed.

yuja added a comment.Thu, Nov 8, 6:27 AM

@@ -464,7 +470,9 @@

		if (iter == NULL)
			return -2;
		while ((iter_item = PyIter_Next(iter))) {
  • iter_item_long = PyInt_AS_LONG(iter_item); + if (!pylong_to_long(iter_item, &iter_item_long)) { + return -1;

Py_DECREF(iter_item) and return -2.

I didn't notice it last time, sorry.

+ }

			Py_DECREF(iter_item);
			if (iter_item_long < min_idx)
				min_idx = iter_item_long;
durin42 updated this revision to Diff 12502.Mon, Nov 12, 11:46 AM
This revision was automatically updated to reflect the committed changes.