unicorn
unicorn copied to clipboard
New and improved Python bindings - pimpl PoC
This is my PoC of pimpl improvements of @elicn's #1629. Thanks for his brilliant work which break down much coupled & nasty code into a much cleaner design! It is really hard to explain my thoughts with just a few sentences and thus I wrote a PoC to show it.
The main contributions are:
- Remove
__new__hijack inUcclass and thusUc(UC_ARCH_XXX, UC_MODE_XXX)indeed returns aUcobject, not a subclass anymore and also allowsUcto be inherited.
I understand it's a tempt to apply factory mode in such case, however, I don't think it's a good practice to return a different instance of what users would like to create, e.g. from Uc to a UcXXX instance. It's slightly subtle that, if the API was originally designed as def Uc(arch, mode) -> [Uc| ...]: then it should be perfectly fine, but for compatibility... kek.
- Apply
pimpldesign on regs and hooks.
It's known that, for a very long time, there are lots of mysteries coupled in both uc_reg_read/write and uc_hook_add. To avoid more code duplication, I would prefer to use pimpl in these two cases. See UcRegXXXImpl and UcHookXXXImpl for details.
Note this is just PoC and some features are not working, like _catch_hook_exception. What's more, I notice that, a few break changes (of course, not all of them are bad!) still exist and I would prefer to put off the merge point to 2.0.1 or even later to collect more feedback as we expect the first version to be more stable and less interruptive to original workflows.
Any comments is highly welcome
You just took a [relatively] clean and tidy design and re-complicated it for no visibile reason..
Like I said in the discussion we had on my PR, the pimpl design does not sit well with Python; it is prehaps more suitable for fundamental programming languages like C++. Regarding Uc instance vs. subclases, I think the statement is wrong: Uc subclasses are still Uc instances and therefore fullfill the expected Uc contract.
As a side note, I think that a new major version release may allow bolder changes and can allow us drop obsolete design choices.
You just took a [relatively] clean and tidy design and re-complicated it for no visibile reason..
As I said previously, I think it's more intuitive to make Uc(XXX, XXX) to return a Uc instead of a subclass, though subclass in principle should act the same as the inherited class.
Like I said in the discussion we had on my PR, the pimpl design does not sit well with Python; it is prehaps more suitable for fundamental programming languages like C++. Regarding Uc instance vs. subclases, I think the statement is wrong: Uc subclasses are still Uc instances and therefore fullfill the expected Uc contract.
Every coin has two sides. Indeed, Python allows you to hijack __new__ for a more flexible way, however, this also applies to our users and thus users might use Uc in any way like my inheriting it (consider users may even do diamond inheritance!). I know, it's still possible to allow users to inherit Uc by hijacking __mro__ but that will make things more tricky. Therefore, I really insist on Uc(xxx, xxx) -> Uc this protocol not broken.
As a side note, I think that a new major version release may allow bolder changes and can allow us drop obsolete design choices.
We are about to release Unicorn2 officially very soon and sorry looks like we lack enough time and users for feedback to avoid potential unexpected break changes or bugs (see my latest comments).
Every coin has two sides. Indeed, Python allows you to hijack
__new__for a more flexible way, however, this also applies to our users and thus users might useUcin any way like my inheriting it (consider users may even do diamond inheritance!). I know, it's still possible to allow users to inheritUcby hijacking__mro__but that will make things more tricky. Therefore, I really insist onUc(xxx, xxx) -> Ucthis protocol not broken.
If you want to allow Uc subclasses you don't need to give up this design. All you need is modify the existing __new__ method a little bit to allow arbitrary Uc subclasses, as I mentioned in my PR. No need to go pimpl just for that.
Every coin has two sides. Indeed, Python allows you to hijack
__new__for a more flexible way, however, this also applies to our users and thus users might useUcin any way like my inheriting it (consider users may even do diamond inheritance!). I know, it's still possible to allow users to inheritUcby hijacking__mro__but that will make things more tricky. Therefore, I really insist onUc(xxx, xxx) -> Ucthis protocol not broken.If you want to allow
Ucsubclasses you don't need to give up this design. All you need is modify the existing__new__method a little bit to allow arbitraryUcsubclasses, as I mentioned in my PR. No need to go pimpl just for that.
What's you proposed approach? Does it make the design even more complex and cause potential break changes?