Dict manager #118
Dict manager #118
Conversation
* Add test for PyDictManager::new_dict * Add test reading from PyDictTracker * Add test writing to PyDictTracker * Add test reading from a default dict * Add test for current_ptr of PyDictTracker * Add more checks on previous tests
| #[getter] | ||
| pub fn get_data(&self, py: Python) -> PyObject { | ||
| PyDictTracker { | ||
| manager: self.manager.clone(), | ||
| key: self.key.clone(), | ||
| } | ||
| .into_py(py) | ||
| } |
There was a problem hiding this comment.
This doesn't look like it's getting any kind of data member. Instead, this is effectively a Python object copy.
There was a problem hiding this comment.
I'm new to the RC structure, but I believe with the manager being an Rc<...>, when I'm cloning the data I'm actually taking the same pointer and increasing the reference counter by one. Am I understanding this correctly?
If the above is true, wouldn't get_data output an object referencing the same tracker in the input , also being able to modify the data inside the original input?
There was a problem hiding this comment.
- Yes,
Rc<...>returns a reference to the same underlying object; - That's true for the field, but it's still creating a new Python object (and in particular it lives on the heap);
- What I'm asking about is more about the type of the return. It's OK that the implementation simply refers to the same
cairo-rsobject, but it doesn't make a lot of sense thatget_datareturns the same Python type I think. I mean, think about the API usage, why would you callget_datato get the samePyDictTrackeryou're calling the method on?
* Add test for invalid key on dict read and write * Add test for get_tracker with invalid dict pointer
Codecov Report
@@ Coverage Diff @@
## main #118 +/- ##
==========================================
+ Coverage 71.40% 72.96% +1.55%
==========================================
Files 12 13 +1
Lines 647 736 +89
==========================================
+ Hits 462 537 +75
- Misses 185 199 +14
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| self.hint_locals.insert( | ||
| "__dict_manager".to_string(), | ||
| PyDictManager::new().into_py(py), | ||
| ); |
There was a problem hiding this comment.
I'd much rather take the GIL only here.
Passing: