unicorn icon indicating copy to clipboard operation
unicorn copied to clipboard

New and improved Python bindings - pimpl PoC

Open wtdcode opened this issue 3 years ago • 4 comments

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 in Uc class and thus Uc(UC_ARCH_XXX, UC_MODE_XXX) indeed returns a Uc object, not a subclass anymore and also allows Uc to 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 pimpl design 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

wtdcode avatar Jun 26 '22 07:06 wtdcode

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.

elicn avatar Jun 26 '22 09:06 elicn

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).

wtdcode avatar Jun 28 '22 13:06 wtdcode

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.

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.

elicn avatar Jun 28 '22 20:06 elicn

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.

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.

What's you proposed approach? Does it make the design even more complex and cause potential break changes?

wtdcode avatar Jun 29 '22 13:06 wtdcode