-
Notifications
You must be signed in to change notification settings - Fork 9
[fix] Null-guard the cling.printValue lookup in op_str #56
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -869,23 +869,30 @@ static PyObject* op_str(CPPInstance* self) { | |
| } | ||
|
|
||
| // 2. Cling's pretty printing (not done through backend for performance | ||
| // reasons) | ||
| // reasons). Cling only: clang-repl has no cling namespace to look up, so the | ||
| // whole path compiles out and str() falls through to the generic repr. | ||
| #ifdef CPPJIT_USE_CLING | ||
| if (!ScopeFlagCheck(self, CPPScope::kNoPrettyPrint)) { | ||
| static PyObject* printValue = nullptr; | ||
| if (!printValue) { | ||
| PyObject* gbl = | ||
| PyDict_GetItemString(PySys_GetObject((char*)"modules"), "cppjit.gbl"); | ||
| PyObject* cl = PyObject_GetAttrString(gbl, (char*)"cling"); | ||
| printValue = PyObject_GetAttrString(cl, (char*)"printValue"); | ||
| Py_DECREF(cl); | ||
| // no cling namespace exists unless user code declares one | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we can just short-circuit this path completely if we know the interpreter is clang-repl, since that lookup would always return null. We could use the compile-time definition
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call — done. The whole pretty-printing block now sits behind |
||
| PyObject* cl = | ||
| gbl ? PyObject_GetAttrString(gbl, (char*)"cling") : nullptr; | ||
| printValue = | ||
| cl ? PyObject_GetAttrString(cl, (char*)"printValue") : nullptr; | ||
| Py_XDECREF(cl); | ||
| // gbl is borrowed | ||
| if (printValue) { | ||
| Py_DECREF(printValue); // make borrowed | ||
| if (!PyCallable_Check(printValue)) | ||
| printValue = nullptr; // unusable ... | ||
| } | ||
| if (!printValue) // unlikely | ||
| if (!printValue) { | ||
| PyErr_Clear(); | ||
| ScopeFlagSet(self, CPPScope::kNoPrettyPrint); | ||
| } | ||
| } | ||
|
|
||
| if (printValue) { | ||
|
|
@@ -929,6 +936,7 @@ static PyObject* op_str(CPPInstance* self) { | |
| // if not available/specialized, don't try again | ||
| ScopeFlagSet(self, CPPScope::kNoPrettyPrint); | ||
| } | ||
| #endif // CPPJIT_USE_CLING | ||
|
|
||
| // 3. Generic printing as done in op_repr | ||
| return op_repr(self); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be required to CppInterOp’s toString interface which will still crash for clang-repl but this time we can fix it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to make that change here: route str() through Cpp::ObjToString and drop the cling-only block. clang-repl would then keep failing in toString until compiler-research/CppInterOp#1100 lands the fix there. Do you want that in this PR, or should we keep the short-circuit and switch once #1100 is fixed?