Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New wrapper for flags #2284

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

Paebbels
Copy link
Member

This PR proposes a solution to get rid of flags_name.value = True syntax and uses proper descriptor protocols for hiding the C-binding wrapper.

Old:

Flags_Gather_Comments.value = True

New:

Flags().Gather_Comments = True

@tgingold if you like the idea, I would do these additional steps:

  • Eliminate the parenthesis (instantiating a singleton):
    Flags.Gather_Comments = True
  • Expand this technique to other flags too. E.g. in parse, vhdl, ...

Originally started PR was #2105.

@tgingold
Copy link
Member

I'd really prefer to have a layered approach. libghdl should be just a ctype interface, and a more pythonic interface would be implemented on top of it.

Could you at least keep the wrappers as globals in flags.py ? That would also keep backward compatibility.
Than I am OK for the Flags class.

@Paebbels
Copy link
Member Author

With my proposal, I would like to enhance what ctypes doesn't do well. They have implemented a descriptor interface, but introduced another level of value access by using .value syntax.

Could you at least keep the wrappers as globals in flags.py

I didn't fully get this point.
Do you mean:

  • keeping the old ctypes wrappers in parallel to the Flags class, OR
  • providing aliases to the descriptors, so old names still work?

My goal is to provide a clean and easy syntax for bare variables from libghdl, which ctype doesn't provide. No further overhead or abstraction is planned on pyGHDL.libghdl.

I'll do further improvements like getting rid of the () and then you can see a more final proposal.


In an ideal world, I wouldn't implement this using a singletone + (instance) properties / descriptors. I would prefer to use class properties (like @Property + @classmethod). Unfortunately, this feature is broken in Python and it's even a vulnerability. I'm trying to push on fixing it in Python itself. If so, it won't be available before 3.12 as I don't expect back porting.

@tgingold
Copy link
Member

tgingold commented Dec 26, 2022 via email

@umarcor
Copy link
Member

umarcor commented Mar 8, 2023

@Paebbels, can this be rebased and merged or does it need additional work?

@Paebbels
Copy link
Member Author

Paebbels commented Mar 8, 2023

I can rebase, but there is additional work needed from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants