Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Py2NRNString. #3249

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Refactor Py2NRNString. #3249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33c746b
Refactor Py2NRNString.
1uc Dec 4, 2024
62ffda0
Missing NRN_EXPORT
1uc Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/neuron/unique_cstr.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#include <utility>
#include <ostream>

#include "nrnpython/nrn_export.hpp"

namespace neuron {

/** A RAII wrapper for C-style strings.
Expand All @@ -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) noexcept {
Expand Down
5 changes: 3 additions & 2 deletions src/nrnpython/inithoc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,9 @@
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);

Check warning on line 118 in src/nrnpython/inithoc.cpp

View check run for this annotation

Codecov / codecov/patch

src/nrnpython/inithoc.cpp#L117-L118 

Added lines #L117 - L118 were not covered by tests
if (strcmp(skey.c_str(), "-print-options") == 0) {
rval = 1;
continue;
Expand Down
49 changes: 24 additions & 25 deletions src/nrnpython/nrnpy_hoc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,21 +419,19 @@
hoc_pushx(nb::cast<double>(po));
} else if (is_python_string(po.ptr())) {
char** ts = hoc_temp_charptr();
Py2NRNString str(po.ptr(), /* disable_release */ true);
if (str.err()) {
auto str = Py2NRNString::as_ascii(po.ptr());
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.ptr(), hocobject_type)) {
// The PyObject_TypeCheck above used to be PyObject_IsInstance. The
Expand Down Expand Up @@ -1042,10 +1040,10 @@

nb::object result;
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 nullptr;
}

Expand Down Expand Up @@ -1433,10 +1431,10 @@
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);
Expand Down Expand Up @@ -2244,9 +2242,10 @@
} 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();
Expand Down Expand Up @@ -2301,10 +2300,11 @@
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);
Expand Down Expand Up @@ -2488,7 +2488,7 @@
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);
Expand Down Expand Up @@ -2755,8 +2755,7 @@
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();

Check warning on line 2758 in src/nrnpython/nrnpy_hoc.cpp

View check run for this annotation

Codecov / codecov/patch

src/nrnpython/nrnpy_hoc.cpp#L2758 

Added line #L2758 was not covered by tests
// TODO: is there a memory leak here? do I need to: s2free.push_back(*ts);
return ts;
}
Expand Down Expand Up @@ -3190,8 +3189,8 @@
return 0;
}

Py2NRNString byteorder(pbo.ptr());
if (byteorder.c_str() == NULL) {
auto byteorder = Py2NRNString::as_ascii(pbo.ptr());
if (!byteorder.is_valid()) {
return 0;
}

Expand Down Expand Up @@ -3295,9 +3294,9 @@
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(

Check warning on line 3299 in src/nrnpython/nrnpy_hoc.cpp

View check run for this annotation

Codecov / codecov/patch

src/nrnpython/nrnpy_hoc.cpp#L3299 

Added line #L3299 was not covered by tests
PyExc_TypeError,
"neuron.coreneuron.nrncore_arg() must return an ascii string");
return nullptr;
Expand Down
43 changes: 22 additions & 21 deletions src/nrnpython/nrnpy_nrn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -1965,10 +1966,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);
Expand Down Expand Up @@ -2025,10 +2026,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()) {
1uc marked this conversation as resolved.
Show resolved Hide resolved
Py2NRNString::set_pyerr(PyExc_TypeError, "attribute name must be a string");
return -1;
}
// printf("section_setattro %s\n", n);
Expand Down Expand Up @@ -2179,10 +2180,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);
Expand Down Expand Up @@ -2322,10 +2323,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);
Expand Down Expand Up @@ -2472,10 +2473,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);
Expand Down Expand Up @@ -2566,10 +2567,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);
Expand Down Expand Up @@ -2619,7 +2620,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;
Expand Down
11 changes: 5 additions & 6 deletions src/nrnpython/nrnpy_p2h.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,8 @@
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);
}
Expand Down Expand Up @@ -231,8 +230,8 @@
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 {
Expand Down Expand Up @@ -363,7 +362,7 @@
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());

Check warning on line 365 in src/nrnpython/nrnpy_p2h.cpp

View check run for this annotation

Codecov / codecov/patch

src/nrnpython/nrnpy_p2h.cpp#L365 

Added line #L365 was not covered by tests
strncpy(buf, str.c_str(), size);
buf[size - 1] = '\0';
} else {
Expand Down
68 changes: 21 additions & 47 deletions src/nrnpython/nrnpy_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,8 @@
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) {
Expand All @@ -36,16 +34,13 @@
} 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()));
Expand All @@ -55,48 +50,27 @@
}
}

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");

Check warning on line 65 in src/nrnpython/nrnpy_utils.cpp

View check run for this annotation

Codecov / codecov/patch

src/nrnpython/nrnpy_utils.cpp#L65 

Added line #L65 was not covered by tests
}
} else {
str_ = strdup("get_pyerr failed at PyErr_Fetch");
str_ = strdup("get_pyerr failed at PyObject_Str");

Check warning on line 68 in src/nrnpython/nrnpy_utils.cpp

View check run for this annotation

Codecov / codecov/patch

src/nrnpython/nrnpy_utils.cpp#L68 

Added line #L68 was not covered by tests
}
} else {
str_ = strdup("get_pyerr failed at PyErr_Fetch");

Check warning on line 71 in src/nrnpython/nrnpy_utils.cpp

View check run for this annotation

Codecov / codecov/patch

src/nrnpython/nrnpy_utils.cpp#L71 

Added line #L71 was not covered by tests
}

PyErr_Clear(); // in case could not turn pvalue into c_str.
return str_;
return neuron::unique_cstr(str_);
}
Loading
Loading