D5235: revlog: replace PyInt_AS_LONG with a more portable helper function
This revision was automatically updated to reflect the committed changes. Closed by commit rHGfa33196088c4: revlog: replace PyInt_AS_LONG with a more portable helper function (authored by durin42, committed by ). CHANGED PRIOR TO COMMIT https://phab.mercurial-scm.org/D5235?vs=12502=12509#toc REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D5235?vs=12502=12509 REVISION DETAIL https://phab.mercurial-scm.org/D5235 AFFECTED FILES mercurial/cext/revlog.c mercurial/cext/util.h CHANGE DETAILS diff --git a/mercurial/cext/util.h b/mercurial/cext/util.h --- a/mercurial/cext/util.h +++ b/mercurial/cext/util.h @@ -58,4 +58,17 @@ 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_ */ diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c --- a/mercurial/cext/revlog.c +++ b/mercurial/cext/revlog.c @@ -24,7 +24,6 @@ #define PyInt_Check PyLong_Check #define PyInt_FromLong PyLong_FromLong #define PyInt_FromSsize_t PyLong_FromSsize_t -#define PyInt_AS_LONG PyLong_AS_LONG #define PyInt_AsLong PyLong_AsLong #endif @@ -161,10 +160,17 @@ int maxrev) { if (rev >= self->length) { + long tmp; PyObject *tuple = PyList_GET_ITEM(self->added, rev - self->length); - ps[0] = (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 5)); - ps[1] = (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 6)); + if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 5), )) { + return -1; + } + ps[0] = (int)tmp; + if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 6), )) { + return -1; + } + ps[1] = (int)tmp; } else { const char *data = index_deref(self, rev); ps[0] = getbe32(data + 24); @@ -464,7 +470,10 @@ 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, _item_long)) { + Py_DECREF(iter_item); + return -2; + } Py_DECREF(iter_item); if (iter_item_long < min_idx) min_idx = iter_item_long; @@ -853,7 +862,11 @@ if (rev >= self->length) { PyObject *tuple = 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), )) { + return -2; + } + return (int)ret; } else { data = index_deref(self, rev); if (data == NULL) { @@ -1384,8 +1397,13 @@ char *node; int rev; - if (PyInt_Check(value)) - return index_get(self, PyInt_AS_LONG(value)); + if (PyInt_Check(value)) { + long idx; + if (!pylong_to_long(value, )) { + return NULL; + } + return index_get(self, idx); + } if (node_check(value, ) == -1) return NULL; @@ -1516,7 +1534,10 @@ char *node; if (PyInt_Check(value)) { - long rev = PyInt_AS_LONG(value); + long rev; + if (!pylong_to_long(value, )) { + return -1; + } return rev >= -1 && rev < index_length(self); } @@ -2404,10 +2425,12 @@ static int rustla_contains(rustlazyancestorsObject *self, PyObject *rev) { - if (!(PyInt_Check(rev))) { + long lrev; + if (!pylong_to_long(rev, )) { + PyErr_Clear(); return 0; } - return rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev)); + return rustlazyancestors_contains(self->iter, lrev); } static PySequenceMethods rustla_sequence_methods = { To: durin42, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5235: revlog: replace PyInt_AS_LONG with a more portable helper function
durin42 updated this revision to Diff 12502. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D5235?vs=12469=12502 REVISION DETAIL https://phab.mercurial-scm.org/D5235 AFFECTED FILES mercurial/cext/revlog.c mercurial/cext/util.h CHANGE DETAILS diff --git a/mercurial/cext/util.h b/mercurial/cext/util.h --- a/mercurial/cext/util.h +++ b/mercurial/cext/util.h @@ -58,4 +58,17 @@ 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_ */ diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c --- a/mercurial/cext/revlog.c +++ b/mercurial/cext/revlog.c @@ -24,7 +24,6 @@ #define PyInt_Check PyLong_Check #define PyInt_FromLong PyLong_FromLong #define PyInt_FromSsize_t PyLong_FromSsize_t -#define PyInt_AS_LONG PyLong_AS_LONG #define PyInt_AsLong PyLong_AsLong #endif @@ -126,7 +125,7 @@ errclass = PyDict_GetItemString(dict, "RevlogError"); if (errclass == NULL) { PyErr_SetString(PyExc_SystemError, - "could not find RevlogError"); + "could not find RevlogError"); goto cleanup; } @@ -146,7 +145,7 @@ if (self->inlined && pos > 0) { if (self->offsets == NULL) { self->offsets = PyMem_Malloc(self->raw_length * -sizeof(*self->offsets)); +sizeof(*self->offsets)); if (self->offsets == NULL) return (const char *)PyErr_NoMemory(); inline_scan(self, self->offsets); @@ -161,10 +160,17 @@ int maxrev) { if (rev >= self->length) { + long tmp; PyObject *tuple = PyList_GET_ITEM(self->added, rev - self->length); - ps[0] = (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 5)); - ps[1] = (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 6)); + if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 5), )) { + return -1; + } + ps[0] = (int)tmp; + if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 6), )) { + return -1; + } + ps[1] = (int)tmp; } else { const char *data = index_deref(self, rev); ps[0] = getbe32(data + 24); @@ -249,8 +255,8 @@ c_node_id = data + 32; entry = Py_BuildValue(tuple_format, offset_flags, comp_len, uncomp_len, - base_rev, link_rev, parent_1, parent_2, c_node_id, - 20); + base_rev, link_rev, parent_1, parent_2, c_node_id, + 20); if (entry) { PyObject_GC_UnTrack(entry); @@ -296,7 +302,7 @@ const char *node = index_node(self, pos); if (node == NULL) { PyErr_Format(PyExc_IndexError, "could not access rev %d", -(int)pos); +(int)pos); } return node; } @@ -464,7 +470,10 @@ 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, _item_long)) { + Py_DECREF(iter_item); + return -2; + } Py_DECREF(iter_item); if (iter_item_long < min_idx) min_idx = iter_item_long; @@ -518,8 +527,8 @@ /* Get arguments */ if (!PyArg_ParseTuple(args, "lO!O!O!", , _Type, , - _Type, , _Type, - )) + _Type, , _Type, + )) goto bail; if (includepatharg == Py_True) @@ -692,7 +701,7 @@ PyList_SET_ITEM(phasessetlist, i + 1, phaseset); if (!PyList_Check(phaseroots)) { PyErr_SetString(PyExc_TypeError, - "roots item must be a list"); + "roots item must be a list"); goto release; }
D5235: revlog: replace PyInt_AS_LONG with a more portable helper function
yuja added a comment. > @@ -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, _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; REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5235 To: durin42, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: D5235: revlog: replace PyInt_AS_LONG with a more portable helper function
> @@ -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, _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; ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5235: revlog: replace PyInt_AS_LONG with a more portable helper function
durin42 added a comment. In https://phab.mercurial-scm.org/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, )) { return 0; > > Needs `PyErr_Clear() since `non_int in self` isn't an error. Good catch, fixed. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5235 To: durin42, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5235: revlog: replace PyInt_AS_LONG with a more portable helper function
durin42 updated this revision to Diff 12469. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D5235?vs=12458=12469 REVISION DETAIL https://phab.mercurial-scm.org/D5235 AFFECTED FILES mercurial/cext/revlog.c mercurial/cext/util.h CHANGE DETAILS diff --git a/mercurial/cext/util.h b/mercurial/cext/util.h --- a/mercurial/cext/util.h +++ b/mercurial/cext/util.h @@ -58,4 +58,17 @@ 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_ */ diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c --- a/mercurial/cext/revlog.c +++ b/mercurial/cext/revlog.c @@ -24,7 +24,6 @@ #define PyInt_Check PyLong_Check #define PyInt_FromLong PyLong_FromLong #define PyInt_FromSsize_t PyLong_FromSsize_t -#define PyInt_AS_LONG PyLong_AS_LONG #define PyInt_AsLong PyLong_AsLong #endif @@ -161,10 +160,17 @@ int maxrev) { if (rev >= self->length) { + long tmp; PyObject *tuple = PyList_GET_ITEM(self->added, rev - self->length); - ps[0] = (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 5)); - ps[1] = (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 6)); + if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 5), )) { + return -1; + } + ps[0] = (int)tmp; + if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 6), )) { + return -1; + } + ps[1] = (int)tmp; } else { const char *data = index_deref(self, rev); ps[0] = getbe32(data + 24); @@ -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, _item_long)) { + return -1; + } Py_DECREF(iter_item); if (iter_item_long < min_idx) min_idx = iter_item_long; @@ -853,7 +861,11 @@ if (rev >= self->length) { PyObject *tuple = 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), )) { + return -2; + } + return (int)ret; } else { data = index_deref(self, rev); if (data == NULL) { @@ -1384,8 +1396,13 @@ char *node; int rev; - if (PyInt_Check(value)) - return index_get(self, PyInt_AS_LONG(value)); + if (PyInt_Check(value)) { + long idx; + if (!pylong_to_long(value, )) { + return NULL; + } + return index_get(self, idx); + } if (node_check(value, ) == -1) return NULL; @@ -1516,7 +1533,10 @@ char *node; if (PyInt_Check(value)) { - long rev = PyInt_AS_LONG(value); + long rev; + if (!pylong_to_long(value, )) { + return -1; + } return rev >= -1 && rev < index_length(self); } @@ -2404,10 +2424,12 @@ static int rustla_contains(rustlazyancestorsObject *self, PyObject *rev) { - if (!(PyInt_Check(rev))) { + long lrev; + if (!pylong_to_long(rev, )) { + PyErr_Clear(); return 0; } - return rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev)); + return rustlazyancestors_contains(self->iter, lrev); } static PySequenceMethods rustla_sequence_methods = { To: durin42, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5235: revlog: replace PyInt_AS_LONG with a more portable helper function
yuja added a comment. > +/* 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, )) { return 0; Needs `PyErr_Clear() since `non_int in self` isn't an error. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5235 To: durin42, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: D5235: revlog: replace PyInt_AS_LONG with a more portable helper function
> +/* 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, )) { > return 0; Needs `PyErr_Clear() since `non_int in self` isn't an error. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D5235: revlog: replace PyInt_AS_LONG with a more portable helper function
durin42 created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION 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. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D5235 AFFECTED FILES mercurial/cext/revlog.c mercurial/cext/util.h CHANGE DETAILS diff --git a/mercurial/cext/util.h b/mercurial/cext/util.h --- a/mercurial/cext/util.h +++ b/mercurial/cext/util.h @@ -58,4 +58,12 @@ 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_ */ diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c --- a/mercurial/cext/revlog.c +++ b/mercurial/cext/revlog.c @@ -24,7 +24,6 @@ #define PyInt_Check PyLong_Check #define PyInt_FromLong PyLong_FromLong #define PyInt_FromSsize_t PyLong_FromSsize_t -#define PyInt_AS_LONG PyLong_AS_LONG #define PyInt_AsLong PyLong_AsLong #endif @@ -161,10 +160,17 @@ int maxrev) { if (rev >= self->length) { + long tmp; PyObject *tuple = PyList_GET_ITEM(self->added, rev - self->length); - ps[0] = (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 5)); - ps[1] = (int)PyInt_AS_LONG(PyTuple_GET_ITEM(tuple, 6)); + if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 5), )) { + return -1; + } + ps[0] = (int)tmp; + if (!pylong_to_long(PyTuple_GET_ITEM(tuple, 6), )) { + return -1; + } + ps[1] = (int)tmp; } else { const char *data = index_deref(self, rev); ps[0] = getbe32(data + 24); @@ -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, _item_long)) { + return -1; + } Py_DECREF(iter_item); if (iter_item_long < min_idx) min_idx = iter_item_long; @@ -853,7 +861,11 @@ if (rev >= self->length) { PyObject *tuple = 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), )) { + return -2; + } + return (int)ret; } else { data = index_deref(self, rev); if (data == NULL) { @@ -1384,8 +1396,13 @@ char *node; int rev; - if (PyInt_Check(value)) - return index_get(self, PyInt_AS_LONG(value)); + if (PyInt_Check(value)) { + long idx; + if (!pylong_to_long(value, )) { + return NULL; + } + return index_get(self, idx); + } if (node_check(value, ) == -1) return NULL; @@ -1516,7 +1533,10 @@ char *node; if (PyInt_Check(value)) { - long rev = PyInt_AS_LONG(value); + long rev; + if (!pylong_to_long(value, )) { + return -1; + } return rev >= -1 && rev < index_length(self); } @@ -2404,10 +2424,11 @@ static int rustla_contains(rustlazyancestorsObject *self, PyObject *rev) { - if (!(PyInt_Check(rev))) { + long lrev; + if (!pylong_to_long(rev, )) { return 0; } - return rustlazyancestors_contains(self->iter, PyInt_AS_LONG(rev)); + return rustlazyancestors_contains(self->iter, lrev); } static PySequenceMethods rustla_sequence_methods = { To: durin42, #hg-reviewers Cc: mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel