This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5235

revlog: replace PyInt_AS_LONG with a more portable helper function
ClosedPublic

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

Download Raw Diff

Details

Reviewers

None

Group Reviewers

hg-reviewers

Commits

rHGfa33196088c4: revlog: replace PyInt_AS_LONG with a more portable helper function

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

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

durin42 created this revision.Nov 6 2018, 11:44 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptNov 6 2018, 11:44 AM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

+/* 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.Nov 7 2018, 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.

@@ -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.Nov 12 2018, 11:46 AM

Closed by commit rHGfa33196088c4: revlog: replace PyInt_AS_LONG with a more portable helper function (authored by durin42). · Explain WhyNov 13 2018, 6:42 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/cext/revlog.c (41 lines)
M			mercurial/cext/util.h (8 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	12458		Nov 6 2018, 11:44 AM	★	★
Diff 2	12469		Nov 7 2018, 1:42 PM	★	★
Diff 3	12502		Nov 12 2018, 11:45 AM	★	★
Diff 4	12509	rHGfa33196088c454ca9154e708c4b4976b139921b4	Nov 6 2018, 11:12 AM	★	★

Commit	Parents	Author	Summary	Date
		Augie Fackler		Nov 6 2018, 11:12 AM

Status	Author	Revision
Closed	durin42	D5235 revlog: replace PyInt_AS_LONG with a more portable helper function
Closed	durin42	D5234 revlog: give formatting to clang-format
Closed	durin42	D5233 revlog: add blank line in comment to help clang-format
Closed	durin42	D5232 revlog: add a comment to help clang-format produce less-awful results

Diff 12458

mercurial/cext/revlog.c


	#ifdef IS_PY3K			#ifdef IS_PY3K
	/* The mapping of Python types is meant to be temporary to get Python			/* The mapping of Python types is meant to be temporary to get Python
	* 3 to compile. We should remove this once Python 3 support is fully			* 3 to compile. We should remove this once Python 3 support is fully
	* supported and proper types are used in the extensions themselves. */			* supported and proper types are used in the extensions themselves. */
	#define PyInt_Check PyLong_Check			#define PyInt_Check PyLong_Check
	#define PyInt_FromLong PyLong_FromLong			#define PyInt_FromLong PyLong_FromLong
	#define PyInt_FromSsize_t PyLong_FromSsize_t			#define PyInt_FromSsize_t PyLong_FromSsize_t
	#define PyInt_AS_LONG PyLong_AS_LONG
	#define PyInt_AsLong PyLong_AsLong			#define PyInt_AsLong PyLong_AsLong
	#endif			#endif

	typedef struct indexObjectStruct indexObject;			typedef struct indexObjectStruct indexObject;

	typedef struct {			typedef struct {
	int children[16];			int children[16];
	} nodetreenode;			} nodetreenode;

	return (const char )(self->buf.buf) + pos v1_hdrsize;			return (const char )(self->buf.buf) + pos v1_hdrsize;
	}			}

	static inline int index_get_parents(indexObject self, Py_ssize_t rev, int ps,			static inline int index_get_parents(indexObject self, Py_ssize_t rev, int ps,
	int maxrev)			int maxrev)
	{			{
	if (rev >= self->length) {			if (rev >= self->length) {
				long tmp;
	PyObject *tuple =			PyObject *tuple =
	PyList_GET_ITEM(self->added, rev - self->length);			PyList_GET_ITEM(self->added, rev - self->length);
	ps[0] = (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 5));			if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 5), &tmp)) {
	ps[1] = (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 6));			return -1;
				}
				ps[0] = (int)tmp;
				if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 6), &tmp)) {
				return -1;
				}
				ps[1] = (int)tmp;
	} else {			} else {
	const char *data = index_deref(self, rev);			const char *data = index_deref(self, rev);
	ps[0] = getbe32(data + 24);			ps[0] = getbe32(data + 24);
	ps[1] = getbe32(data + 28);			ps[1] = getbe32(data + 28);
	}			}
	/* If index file is corrupted, ps[] may point to invalid revisions. So			/* If index file is corrupted, ps[] may point to invalid revisions. So
	* there is a risk of buffer overflow to trust them unconditionally. */			* there is a risk of buffer overflow to trust them unconditionally. */
	if (ps[0] > maxrev \|\| ps[1] > maxrev) {			if (ps[0] > maxrev \|\| ps[1] > maxrev) {
	Py_ssize_t min_idx = index_length(self) + 2;			Py_ssize_t min_idx = index_length(self) + 2;
	long iter_item_long;			long iter_item_long;

	if (PyList_GET_SIZE(list) != 0) {			if (PyList_GET_SIZE(list) != 0) {
	iter = PyObject_GetIter(list);			iter = PyObject_GetIter(list);
	if (iter == NULL)			if (iter == NULL)
	return -2;			return -2;
	while ((iter_item = PyIter_Next(iter))) {			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);			Py_DECREF(iter_item);
	if (iter_item_long < min_idx)			if (iter_item_long < min_idx)
	min_idx = iter_item_long;			min_idx = iter_item_long;
	phases[iter_item_long] = (char)marker;			phases[iter_item_long] = (char)marker;
	}			}
	Py_DECREF(iter);			Py_DECREF(iter);
	}			}

	*/			*/
	static inline int index_baserev(indexObject *self, int rev)			static inline int index_baserev(indexObject *self, int rev)
	{			{
	const char *data;			const char *data;

	if (rev >= self->length) {			if (rev >= self->length) {
	PyObject *tuple =			PyObject *tuple =
	PyList_GET_ITEM(self->added, rev - self->length);			PyList_GET_ITEM(self->added, rev - self->length);
	return (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 3));			long ret;
				if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 3), &ret)) {
				return -2;
				}
				return (int)ret;
	} else {			} else {
	data = index_deref(self, rev);			data = index_deref(self, rev);
	if (data == NULL) {			if (data == NULL) {
	return -2;			return -2;
	}			}

	return getbe32(data + 16);			return getbe32(data + 16);
	}			}
	return -2;			return -2;
	}			}

	static PyObject index_getitem(indexObject self, PyObject *value)			static PyObject index_getitem(indexObject self, PyObject *value)
	{			{
	char *node;			char *node;
	int rev;			int rev;

	if (PyInt_Check(value))			if (PyInt_Check(value)) {
	return index_get(self, PyInt_AS_LONG(value));			long idx;
				if (!pylong_to_long(value, &idx)) {
				return NULL;
				}
				return index_get(self, idx);
				}

	if (node_check(value, &node) == -1)			if (node_check(value, &node) == -1)
	return NULL;			return NULL;
	rev = index_find_node(self, node, 20);			rev = index_find_node(self, node, 20);
	if (rev >= -1)			if (rev >= -1)
	return PyInt_FromLong(rev);			return PyInt_FromLong(rev);
	if (rev == -2)			if (rev == -2)
	raise_revlog_error();			raise_revlog_error();
	return PyInt_FromLong(rev);			return PyInt_FromLong(rev);
	}			}

	static int index_contains(indexObject self, PyObject value)			static int index_contains(indexObject self, PyObject value)
	{			{
	char *node;			char *node;

	if (PyInt_Check(value)) {			if (PyInt_Check(value)) {
	long rev = PyInt_AS_LONG(value);			long rev;
				if (!pylong_to_long(value, &rev)) {
				return -1;
				}
	return rev >= -1 && rev < index_length(self);			return rev >= -1 && rev < index_length(self);
	}			}

	if (node_check(value, &node) == -1)			if (node_check(value, &node) == -1)
	return -1;			return -1;

	switch (index_find_node(self, node, 20)) {			switch (index_find_node(self, node, 20)) {
	case -3:			case -3:
	*/			*/
	return NULL;			return NULL;
	}			}
	return PyInt_FromLong(res);			return PyInt_FromLong(res);
	}			}

	static int rustla_contains(rustlazyancestorsObject self, PyObject rev)			static int rustla_contains(rustlazyancestorsObject self, PyObject rev)
	{			{
	if (!(PyInt_Check(rev))) {			long lrev;
				if (!pylong_to_long(rev, &lrev)) {
	return 0;			return 0;
	}			}
	return rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev));			return rustlazyancestors_contains(self->iter, lrev);
	}			}

	static PySequenceMethods rustla_sequence_methods = {			static PySequenceMethods rustla_sequence_methods = {
	0, /* sq_length */			0, /* sq_length */
	0, /* sq_concat */			0, /* sq_concat */
	0, /* sq_repeat */			0, /* sq_repeat */
	0, /* sq_item */			0, /* sq_item */
	0, /* sq_slice */			0, /* sq_slice */

mercurial/cext/util.h

	parameter. For example, with the initial minused = 1000, the			parameter. For example, with the initial minused = 1000, the
	dictionary created has size 1024. Of course in a lot of cases that			dictionary created has size 1024. Of course in a lot of cases that
	can be greater than the maximum load factor Python's dict object			can be greater than the maximum load factor Python's dict object
	expects (= 2/3), so as soon as we cross the threshold we'll resize			expects (= 2/3), so as soon as we cross the threshold we'll resize
	anyway. So create a dictionary that's at least 3/2 the size. */			anyway. So create a dictionary that's at least 3/2 the size. */
	return _PyDict_NewPresized(((1 + expected_size) / 2) * 3);			return _PyDict_NewPresized(((1 + expected_size) / 2) * 3);
	}			}

				/* 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)
				{
				*out = PyLong_AsLong(pylong);
				return PyErr_Occurred() == NULL;
				}

	#endif /* _HG_UTIL_H_ */			#endif /* _HG_UTIL_H_ */