Now uniondatapackstore can also hold python data stores. PythonDataStore
wrapper simply passes function calls to underlying python objects and marshals
the output.
Details
Diff Detail
- Repository
- rFBHGX Facebook Mercurial Extensions
- Branch
- pythonstore (bookmark) on default (branch)
- Lint
Lint OK - Unit
Unit Tests OK
Event Timeline
And testing plan is not exhaustive of course.
cstore/datastore.h | ||
---|---|---|
16 | I think that looks ugly. Perhaps we should make a BaseDeltaChain class and extend CDeltaChain and PythonDeltaChain from it? | |
cstore/py-pythondatastore.h | ||
2 | This of course is not a header file. But, I am trying to separate python specific code from non python specific code, because of 'Eden related stuff'. | |
cstore/uniondatapackstore.cpp | ||
34 | I dont like it either because it violates Liskov substitution principle, but we need to do memory management for pythondatastores. Perhaps there is a better way? |
I only read part of it. Will add more comments later.
cstore/datastore.h | ||
---|---|---|
16 | Hmm, yea, given all the if statements I think that makes sense. | |
23 | Does this work? How does sizeof know the size of filename? The only way I can think of is by walking until the first \0 character, which won't work for delta. My guess is that sizeof does a compile time lookup of the size of the compile time type, and therefore doesn't actually return the string length. I would pass the size's in directly. | |
cstore/uniondatapackstore.cpp | ||
34 | UnionDatapackStore doesn't own the lifetime of normal cdatapack stores because they were created initially as a Python object and and therefore kept alive by a reference from py_uniondatapackstore. Maybe we just follow the same pattern and add a std::vector<PythonObj> pythonsubstores member next to substores on py_uniondatapackstore in py-structs.h. So they'll have the same lifetime structure and we can get rid of this code here. |
Extended CDeltaChain and PyDeltaChain from base DeltaChain. Updated memory management for PythonDataStores.
Pass filename and delta size directly to DeltaChainLink costructor. Refactored the code.
Needs to be split into at least three -- two refactors and then the code you're actually adding. A few inline comments. Will review more in depth after you've split it up.
cstore/datastore.h | ||
---|---|---|
21 | Split this refactor into a smaller commit below this one. Try to avoid refactors combined with new code -- it makes everything much harder to review. | |
cstore/py-datapackstore.h | ||
112–140 | Should be split into a separate commit to make this one more reviewable. The first commit can move this and show everything still works, then future commits can implement the new/additional functionality. | |
218 | I'd name this better -- perhaps is_datapack? | |
220 | No need to do == 1 here, unless IsInstance can return something other than 1 and 0, in which case this would warrant a comment. |
cstore/py-datapackstore.h | ||
---|---|---|
220 | In fact yes, it also can return -1 in case of an error, which I missed |
I think that looks ugly. Perhaps we should make a BaseDeltaChain class and extend CDeltaChain and PythonDeltaChain from it?