Add missing nullptr checks in atomlist #220
Conversation
|
Thanks can you rebase to get the formatter happy. |
| @@ -785,16 +785,26 @@ class AtomCListHandler : public AtomListHandler | |||
| { | |||
| static char *kwlist[] = { "key", "reverse", 0 }; | |||
| // Get a reference to builtins (borrowed ref hence the incref) | |||
| cppy::ptr builtins( cppy::incref( PyImport_AddModule("builtins") ) ); | |||
| cppy::ptr builtins( cppy::xincref( PyImport_AddModule("builtins") ) ); | |||
There was a problem hiding this comment.
why are we doingAddModule here instead of Import? If we need a builtin function from globals, that should be pre-fetched on module initialization.
There was a problem hiding this comment.
I do not remember the details but I likely followed an example. I would like to fully move away from global static and properly use module state (and we could store super there) but I have no idea if I will find the time to do so.
There was a problem hiding this comment.
But hold on here. We seem to have deviated quite a bit from the original code, by calling a builtin and returning before the observer code. What exactly is going on, and why have we deviated so much from the original code?
There was a problem hiding this comment.
Once again I do not have the whole history in mind but I think we were relying on a private function to sort the list that disappeared. However I do not remember why I went with super rather than looking up the method directly on list. It may have just been simpler.
MatthieuDartiailh
force-pushed
the
atomlist-nullchecks
branch
from
fcfaa18 to
f006704
Compare
|
I will be worth revisiting this in the future to see if we can improve but the changes here make the code more correct and there is no reason to delay them so I will merge once the CI is green. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #220 +/- ##
=======================================
Coverage 97.67% 97.67%
=======================================
Files 24 24
Lines 1074 1074
Branches 162 162
=======================================
Hits 1049 1049
Misses 12 12
Partials 13 13 |
|
Now that this is merged, I think we need to avoid going thru |
No description provided.