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.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durin42 created this revision.Nov 6 2018, 11:44 AM
yuja added a subscriber: yuja.Nov 7 2018, 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.Nov 7 2018, 1:42 PM
durin42 added a comment.Nov 7 2018, 1:44 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.Nov 8 2018, 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.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

PathPackages
Mmercurial/cext/revlog.c (43 lines)
Mmercurial/cext/util.h (13 lines)

Diff 12509

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)) {
Py_DECREF(iter_item);
return -2;
}
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)) {
PyErr_Clear();
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);
/* Fast path to avoid hitting PyErr_Occurred if the value was obviously
* not an error. */
if (*out != -1) {
return true;
}
return PyErr_Occurred() == NULL;
}
#endif /* _HG_UTIL_H_ */ #endif /* _HG_UTIL_H_ */