From 9d6872b019b3aae6d64f6f56b4f7771af26f9e61 Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Tue, 26 Nov 2024 09:16:33 +0100 Subject: [PATCH] Refactor Py2NRNString. --- src/neuron/unique_cstr.hpp | 4 ++- src/nrnpython/inithoc.cpp | 5 +-- src/nrnpython/nrnpy_hoc.cpp | 47 ++++++++++++------------ src/nrnpython/nrnpy_nrn.cpp | 43 +++++++++++----------- src/nrnpython/nrnpy_p2h.cpp | 11 +++--- src/nrnpython/nrnpy_utils.cpp | 68 +++++++++++------------------------ src/nrnpython/nrnpy_utils.h | 61 +++++-------------------------- 7 files changed, 85 insertions(+), 154 deletions(-) diff --git a/src/neuron/unique_cstr.hpp b/src/neuron/unique_cstr.hpp index 7b5bf9ad7e..3b498fb5e7 100644 --- a/src/neuron/unique_cstr.hpp +++ b/src/neuron/unique_cstr.hpp @@ -4,6 +4,8 @@ #include #include +#include "nrnpython/nrn_export.hpp" + namespace neuron { /** A RAII wrapper for C-style strings. @@ -13,7 +15,7 @@ namespace neuron { * ownership, this is achieved using `.release()`, which returns the contained C-string and makes * this object invalid. */ -class unique_cstr { +class NRN_EXPORT unique_cstr { public: unique_cstr(const unique_cstr&) = delete; unique_cstr(unique_cstr&& other) { diff --git a/src/nrnpython/inithoc.cpp b/src/nrnpython/inithoc.cpp index 9e9c9ef508..73111db2a1 100644 --- a/src/nrnpython/inithoc.cpp +++ b/src/nrnpython/inithoc.cpp @@ -113,8 +113,9 @@ static int add_neuron_options() { PySys_WriteStdout("A neuron_options key:value is not a string:string or string:None\n"); continue; } - Py2NRNString skey(key); - Py2NRNString sval(value); + + auto skey = Py2NRNString::as_ascii(key); + auto sval = Py2NRNString::as_ascii(value); if (strcmp(skey.c_str(), "-print-options") == 0) { rval = 1; continue; diff --git a/src/nrnpython/nrnpy_hoc.cpp b/src/nrnpython/nrnpy_hoc.cpp index 26e683a78a..803736e222 100644 --- a/src/nrnpython/nrnpy_hoc.cpp +++ b/src/nrnpython/nrnpy_hoc.cpp @@ -420,21 +420,19 @@ int hocobj_pushargs(PyObject* args, std::vector& s2free) { hoc_pushx(static_cast(pn)); } else if (is_python_string(po)) { char** ts = hoc_temp_charptr(); - Py2NRNString str(po, /* disable_release */ true); - if (str.err()) { + auto str = Py2NRNString::as_ascii(po); + if (!str.is_valid()) { // Since Python error has been set, need to clear, or hoc_execerror // printing with Printf will generate a // Exception ignored on calling ctypes callback function. // So get the message, clear, and make the message // part of the execerror. - auto err = neuron::unique_cstr(str.get_pyerr()); - *ts = err.c_str(); - s2free.push_back(std::move(err)); + auto err = Py2NRNString::get_pyerr(); hoc_execerr_ext("python string arg cannot decode into c_str. Pyerr message: %s", - *ts); + err.c_str()); } *ts = str.c_str(); - s2free.push_back(neuron::unique_cstr(*ts)); + s2free.push_back(std::move(str)); hoc_pushstr(ts); } else if (PyObject_TypeCheck(po, hocobject_type)) { // The PyObject_TypeCheck above used to be PyObject_IsInstance. The @@ -1034,10 +1032,10 @@ static PyObject* hocobj_getattr(PyObject* subself, PyObject* pyname) { PyObject* result = NULL; int isptr = 0; - Py2NRNString name(pyname); + auto name = Py2NRNString::as_ascii(pyname); char* n = name.c_str(); if (!n) { - name.set_pyerr(PyExc_TypeError, "attribute name must be a string"); + Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string"); return NULL; } @@ -1425,10 +1423,10 @@ static int hocobj_setattro(PyObject* subself, PyObject* pyname, PyObject* value) if (self->type_ == PyHoc::HocObject && !self->ho_) { return 1; } - Py2NRNString name(pyname); + auto name = Py2NRNString::as_ascii(pyname); char* n = name.c_str(); if (!n) { - name.set_pyerr(PyExc_TypeError, "attribute name must be a string"); + Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string"); return -1; } // printf("hocobj_setattro %s\n", n); @@ -2228,9 +2226,10 @@ static PyObject* mkref(PyObject* self, PyObject* args) { } else if (is_python_string(pa)) { result->type_ = PyHoc::HocRefStr; result->u.s_ = 0; - Py2NRNString str(pa); - if (str.err()) { - str.set_pyerr(PyExc_TypeError, "string arg must have only ascii characters"); + auto str = Py2NRNString::as_ascii(pa); + if (!str.is_valid()) { + Py2NRNString::set_pyerr(PyExc_TypeError, + "string arg must have only ascii characters"); return NULL; } char* cpa = str.c_str(); @@ -2283,10 +2282,11 @@ static PyObject* setpointer(PyObject* self, PyObject* args) { if (hpp->type_ != PyHoc::HocObject) { goto done; } - Py2NRNString str(name); + auto str = Py2NRNString::as_ascii(name); char* n = str.c_str(); - if (str.err()) { - str.set_pyerr(PyExc_TypeError, "POINTER name can contain only ascii characters"); + if (!str.is_valid()) { + Py2NRNString::set_pyerr(PyExc_TypeError, + "POINTER name can contain only ascii characters"); return NULL; } Symbol* sym = getsym(n, hpp->ho_, 0); @@ -2470,7 +2470,7 @@ static char* double_array_interface(PyObject* po, long& stride) { PyObject* psize; if (PyObject_HasAttrString(po, "__array_interface__")) { auto ai = nb::steal(PyObject_GetAttrString(po, "__array_interface__")); - Py2NRNString typestr(PyDict_GetItemString(ai.ptr(), "typestr")); + auto typestr = Py2NRNString::as_ascii(PyDict_GetItemString(ai.ptr(), "typestr")); if (strcmp(typestr.c_str(), array_interface_typestr) == 0) { data = PyLong_AsVoidPtr(PyTuple_GetItem(PyDict_GetItemString(ai.ptr(), "data"), 0)); // printf("double_array_interface idata = %ld\n", idata); @@ -2736,8 +2736,7 @@ static char** gui_helper_3_str_(const char* name, Object* obj, int handle_strptr if (gui_callback) { auto po = nb::steal(gui_helper_3_helper_(name, obj, handle_strptr)); char** ts = hoc_temp_charptr(); - Py2NRNString str(po.ptr(), true); - *ts = str.c_str(); + *ts = Py2NRNString::as_ascii(po.ptr()).release(); // TODO: is there a memory leak here? do I need to: s2free.push_back(*ts); return ts; } @@ -3166,7 +3165,7 @@ char get_endian_character() { return 0; } - Py2NRNString byteorder(pbo); + auto byteorder = Py2NRNString::as_ascii(pbo); if (byteorder.c_str() == NULL) { return 0; } @@ -3271,9 +3270,9 @@ static char* nrncore_arg(double tstop) { if (ts) { auto arg = nb::steal(PyObject_CallObject(callable.ptr(), ts.ptr())); if (arg) { - Py2NRNString str(arg.ptr()); - if (str.err()) { - str.set_pyerr( + auto str = Py2NRNString::as_ascii(arg.ptr()); + if (!str.is_valid()) { + Py2NRNString::set_pyerr( PyExc_TypeError, "neuron.coreneuron.nrncore_arg() must return an ascii string"); return nullptr; diff --git a/src/nrnpython/nrnpy_nrn.cpp b/src/nrnpython/nrnpy_nrn.cpp index d5aaab3a64..cf1e875d2f 100644 --- a/src/nrnpython/nrnpy_nrn.cpp +++ b/src/nrnpython/nrnpy_nrn.cpp @@ -409,9 +409,10 @@ static int NPySecObj_init(NPySecObj* self, PyObject* args, PyObject* kwds) { Py_XDECREF(self->cell_weakref_); return -1; } - Py2NRNString str(cell_str.ptr()); - if (str.err()) { - str.set_pyerr(PyExc_TypeError, "cell name contains non ascii character"); + auto str = Py2NRNString::as_ascii(cell_str.ptr()); + if (!str.is_valid()) { + Py2NRNString::set_pyerr(PyExc_TypeError, + "cell name contains non ascii character"); Py_XDECREF(self->cell_weakref_); return -1; } @@ -1947,10 +1948,10 @@ static PyObject* section_getattro(NPySecObj* self, PyObject* pyname) { CHECK_SEC_INVALID(sec); PyObject* rv; auto _pyname_tracker = nb::borrow(pyname); // keep refcount+1 during use - Py2NRNString name(pyname); + auto name = Py2NRNString::as_ascii(pyname); char* n = name.c_str(); - if (name.err()) { - name.set_pyerr(PyExc_TypeError, "attribute name must be a string"); + if (!name.is_valid()) { + Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string"); return nullptr; } // printf("section_getattr %s\n", n); @@ -2007,10 +2008,10 @@ static int section_setattro(NPySecObj* self, PyObject* pyname, PyObject* value) PyObject* rv; int err = 0; auto _pyname_tracker = nb::borrow(pyname); // keep refcount+1 during use - Py2NRNString name(pyname); + auto name = Py2NRNString::as_ascii(pyname); char* n = name.c_str(); - if (name.err()) { - name.set_pyerr(PyExc_TypeError, "attribute name must be a string"); + if (!name.is_valid()) { + Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string"); return -1; } // printf("section_setattro %s\n", n); @@ -2160,10 +2161,10 @@ static PyObject* segment_getattro(NPySegObj* self, PyObject* pyname) { Symbol* sym; auto _pyname_tracker = nb::borrow(pyname); // keep refcount+1 during use - Py2NRNString name(pyname); + auto name = Py2NRNString::as_ascii(pyname); char* n = name.c_str(); - if (name.err()) { - name.set_pyerr(PyExc_TypeError, "attribute name must be a string"); + if (!name.is_valid()) { + Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string"); return nullptr; } // printf("segment_getattr %s\n", n); @@ -2302,10 +2303,10 @@ static int segment_setattro(NPySegObj* self, PyObject* pyname, PyObject* value) Symbol* sym; int err = 0; auto _pyname_tracker = nb::borrow(pyname); // keep refcount+1 during use - Py2NRNString name(pyname); + auto name = Py2NRNString::as_ascii(pyname); char* n = name.c_str(); - if (name.err()) { - name.set_pyerr(PyExc_TypeError, "attribute name must be a string"); + if (!name.is_valid()) { + Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string"); return -1; } // printf("segment_setattro %s\n", n); @@ -2451,10 +2452,10 @@ static PyObject* mech_getattro(NPyMechObj* self, PyObject* pyname) { CHECK_SEC_INVALID(sec) CHECK_PROP_INVALID(self->prop_id_); auto _pyname_tracker = nb::borrow(pyname); // keep refcount+1 during use - Py2NRNString name(pyname); + auto name = Py2NRNString::as_ascii(pyname); char* n = name.c_str(); if (!n) { - name.set_pyerr(PyExc_TypeError, "attribute name must be a string"); + Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string"); return nullptr; } // printf("mech_getattro %s\n", n); @@ -2545,10 +2546,10 @@ static int mech_setattro(NPyMechObj* self, PyObject* pyname, PyObject* value) { int err = 0; auto _pyname_tracker = nb::borrow(pyname); // keep refcount+1 during use - Py2NRNString name(pyname); + auto name = Py2NRNString::as_ascii(pyname); char* n = name.c_str(); - if (name.err()) { - name.set_pyerr(PyExc_TypeError, "attribute name must be a string"); + if (!name.is_valid()) { + Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string"); return -1; } // printf("mech_setattro %s\n", n); @@ -2598,7 +2599,7 @@ neuron::container::generic_data_handle* nrnpy_setpointer_helper(PyObject* pyname NPyMechObj* m = (NPyMechObj*) mech; Symbol* msym = memb_func[m->type_].sym; char buf[200]; - Py2NRNString name(pyname); + auto name = Py2NRNString::as_ascii(pyname); char* n = name.c_str(); if (!n) { return nullptr; diff --git a/src/nrnpython/nrnpy_p2h.cpp b/src/nrnpython/nrnpy_p2h.cpp index 7f3938d320..d85aed2de7 100644 --- a/src/nrnpython/nrnpy_p2h.cpp +++ b/src/nrnpython/nrnpy_p2h.cpp @@ -172,9 +172,8 @@ static void py2n_component(Object* ob, Symbol* sym, int nindex, int isfunc) { for (i = 0; i < nindex; ++i) { nb::object arg = nb::steal(nrnpy_hoc_pop("isfunc py2n_component")); if (!arg) { - PyErr2NRNString e; - e.get_pyerr(); - hoc_execerr_ext("arg %d error: %s", i, e.c_str()); + auto err = Py2NRNString::get_pyerr(); + hoc_execerr_ext("arg %d error: %s", i, err.c_str()); } args.append(arg); } @@ -231,8 +230,8 @@ static void py2n_component(Object* ob, Symbol* sym, int nindex, int isfunc) { hoc_pushx(d); } else if (is_python_string(result.ptr())) { char** ts = hoc_temp_charptr(); - Py2NRNString str(result.ptr(), true); - *ts = str.c_str(); + // TODO double check that this doesn't leak. + *ts = Py2NRNString::as_ascii(result.ptr()).release(); hoc_pop_defer(); hoc_pushstr(ts); } else { @@ -363,7 +362,7 @@ static int hoccommand_exec_strret(Object* ho, char* buf, int size) { nb::object r = hoccommand_exec_help(ho); if (r.is_valid()) { nb::str pn(r); - Py2NRNString str(pn.ptr()); + auto str = Py2NRNString::as_ascii(pn.ptr()); strncpy(buf, str.c_str(), size); buf[size - 1] = '\0'; } else { diff --git a/src/nrnpython/nrnpy_utils.cpp b/src/nrnpython/nrnpy_utils.cpp index 2ea39867fd..44b116e977 100644 --- a/src/nrnpython/nrnpy_utils.cpp +++ b/src/nrnpython/nrnpy_utils.cpp @@ -14,10 +14,8 @@ inline std::tuple fetch_pyerr() { return std::make_tuple(nb::steal(ptype), nb::steal(pvalue), nb::steal(ptraceback)); } - -Py2NRNString::Py2NRNString(PyObject* python_string, bool disable_release) { - disable_release_ = disable_release; - str_ = NULL; +neuron::unique_cstr Py2NRNString::as_ascii(PyObject* python_string) { + char* str_ = nullptr; if (PyUnicode_Check(python_string)) { auto py_bytes = nb::steal(PyUnicode_AsASCIIString(python_string)); if (py_bytes) { @@ -36,16 +34,13 @@ Py2NRNString::Py2NRNString(PyObject* python_string, bool disable_release) { } else { // Neither Unicode or PyBytes PyErr_SetString(PyExc_TypeError, "Neither Unicode or PyBytes"); } + + return neuron::unique_cstr(str_); } void Py2NRNString::set_pyerr(PyObject* type, const char* message) { - nb::object err_type; - nb::object err_value; - nb::object err_traceback; + auto [err_type, err_value, err_traceback] = fetch_pyerr(); - if (err()) { - std::tie(err_type, err_value, err_traceback) = fetch_pyerr(); - } if (err_value && err_type) { auto umes = nb::steal( PyUnicode_FromFormat("%s (Note: %S: %S)", message, err_type.ptr(), err_value.ptr())); @@ -55,48 +50,27 @@ void Py2NRNString::set_pyerr(PyObject* type, const char* message) { } } -char* Py2NRNString::get_pyerr() { - if (err()) { - auto [ptype, pvalue, ptraceback] = fetch_pyerr(); - if (pvalue) { - auto pstr = nb::steal(PyObject_Str(pvalue.ptr())); - if (pstr) { - const char* err_msg = PyUnicode_AsUTF8(pstr.ptr()); - if (err_msg) { - str_ = strdup(err_msg); - } else { - str_ = strdup("get_pyerr failed at PyUnicode_AsUTF8"); - } - } else { - str_ = strdup("get_pyerr failed at PyObject_Str"); - } - } else { - str_ = strdup("get_pyerr failed at PyErr_Fetch"); - } - } - PyErr_Clear(); // in case could not turn pvalue into c_str. - return str_; -} +neuron::unique_cstr Py2NRNString::get_pyerr() { + // Must be called after an error happend. + char* str_ = nullptr; -char* PyErr2NRNString::get_pyerr() { - if (PyErr_Occurred()) { - auto [ptype, pvalue, ptraceback] = fetch_pyerr(); - if (pvalue) { - auto pstr = nb::steal(PyObject_Str(pvalue.ptr())); - if (pstr) { - const char* err_msg = PyUnicode_AsUTF8(pstr.ptr()); - if (err_msg) { - str_ = strdup(err_msg); - } else { - str_ = strdup("get_pyerr failed at PyUnicode_AsUTF8"); - } + auto [ptype, pvalue, ptraceback] = fetch_pyerr(); + if (pvalue) { + auto pstr = nb::steal(PyObject_Str(pvalue.ptr())); + if (pstr) { + const char* err_msg = PyUnicode_AsUTF8(pstr.ptr()); + if (err_msg) { + str_ = strdup(err_msg); } else { - str_ = strdup("get_pyerr failed at PyObject_Str"); + str_ = strdup("get_pyerr failed at PyUnicode_AsUTF8"); } } else { - str_ = strdup("get_pyerr failed at PyErr_Fetch"); + str_ = strdup("get_pyerr failed at PyObject_Str"); } + } else { + str_ = strdup("get_pyerr failed at PyErr_Fetch"); } + PyErr_Clear(); // in case could not turn pvalue into c_str. - return str_; + return neuron::unique_cstr(str_); } diff --git a/src/nrnpython/nrnpy_utils.h b/src/nrnpython/nrnpy_utils.h index bdfaa28896..60da89489c 100644 --- a/src/nrnpython/nrnpy_utils.h +++ b/src/nrnpython/nrnpy_utils.h @@ -2,6 +2,7 @@ #include "nrnwrap_Python.h" #include "nrn_export.hpp" +#include "neuron/unique_cstr.hpp" #include @@ -9,63 +10,17 @@ inline bool is_python_string(PyObject* python_string) { return PyUnicode_Check(python_string) || PyBytes_Check(python_string); } -class NRN_EXPORT Py2NRNString { +class Py2NRNString { public: - Py2NRNString(PyObject* python_string, bool disable_release = false); + [[nodiscard]] static neuron::unique_cstr as_ascii(PyObject* python_string); - ~Py2NRNString() { - if (!disable_release_ && str_) { - free(str_); - } - } - inline char* c_str() const { - return str_; - } - inline bool err() const { - return str_ == NULL; - } - - void set_pyerr(PyObject* type, const char* message); - char* get_pyerr(); + static void set_pyerr(PyObject* type, const char* message); + [[nodiscard]] static neuron::unique_cstr get_pyerr(); private: - Py2NRNString(); - Py2NRNString(const Py2NRNString&); - Py2NRNString& operator=(const Py2NRNString&); - - char* str_; - bool disable_release_; -}; - -/** @brief For when hoc_execerror must handle the Python error. - * Idiom: PyErr2NRNString e; - * -- clean up any python objects -- - * hoc_execerr_ext("hoc message : %s", e.c_str()); - * e will be automatically deleted even though execerror does not return. - */ -class NRN_EXPORT PyErr2NRNString { - public: - PyErr2NRNString() { - str_ = NULL; - } - - ~PyErr2NRNString() { - if (str_) { - free(str_); - } - } - - inline char* c_str() const { - return str_; - } - - char* get_pyerr(); - - private: - PyErr2NRNString(const PyErr2NRNString&); - PyErr2NRNString& operator=(const PyErr2NRNString&); - - char* str_; + Py2NRNString() = delete; + Py2NRNString(const Py2NRNString&) = delete; + Py2NRNString& operator=(const Py2NRNString&) = delete; }; extern void nrnpy_sec_referr();