-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Refactor and simplify the headers in hpy/devel/include #225
Conversation
…universal/*.h and cpython/*.h
…y.h and avoid confusion
When it was introduced, it was meant to be different in cpython and universal cases. But: 1) it's not very clear in which cases it's needed to use it 2) nowadays it's the same on both ABI modes Kill it and use _HPy_HIDDEN directly, I think it's clearer
…C} in hpy.h. More refactoring needed
The current way to implement things like HPy_SetItem_i&co. is overly complicated: we try to reuse the implementation on Universal and CPython by using the ugly _HPy_IMPL_NAME and _HPy_IMPL_NAME_NOPREFIX macros, but there is a simpler way, which is already used for many other functions: write them directly as ctx_* and manually write the HPy_* wrapper for the CPython ABI case. Eventually, the HPy_* wrappers should be autogenerated, though.
…d of having a zillion of different files, each one containing only one or two prototypes
We can finally kill the ugly _HPy_IMPL_NAME / _HPy_IMPL_NAME_NOPREFIX hack. Instead of relying on the macros to generate two different things from a shared autogen_impl, we can simply autogen two different files, one for CPython and one for Universal ABI. Much simpler and easier to read than before.
… while, we should run these tests in CI :(
…ier to turn it on temporarly when needed
Having two different definitions of HPyAPI_STORAGE was useful when implementation.h was shared between hpy/universal and hpy/cpython but now that they are separate it is confusing to have a single macro for two different things. It's much easiery to clearly separate the roles of ctx_* functions and HPy_* ones.
- PyPy: ._i is an index into a list | ||
|
||
- GraalPython: ??? | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start on reorganizing to me. I left some questions / comments, but am +1 on the changes overall.
thank you! I'll wait until we reach a consensus on the |
…ior of all the HPyAPI_* variants
The current headers are a mess, this PR tries to simplify/clean/improve the situation.
Major points:
Less pollution of the
#include
"namespace". On master we put in the main include pathhpy.h
,common/
,cpython/
anduniversal/
. Now we only havehpy.h
andhpy/
Unify the Universal and CPython headers as much as possible. On master we have
hpy.h
,universal/hpy.h
andcpython/hpy.h
, and the latter two contains many duplicate or almost duplicate code. Now we have a singlehpy.h
which contains a lot of shared definitions, and#includes
the appropriate files fromhpy/universal/*.h
andhpy/cpython/*.h
as needed.Kill the ugly hacks around
_HPy_IMPL_NAME
andimplementation.h
. On master,implementation.h
is shared between the CPython and Universal modes and we use macro tricks to adapt it to both cases. But it turns out that the vast majority of it is autogenerated, and so it's much easier to autogenerate two different versions directly. There were also a few non-autogenerated functions in the oldimplementation.h
but it was weird anyway: we already have a way to sharectx_*
implementations betweent he two ABI modes, so we can just use it also in this case.HPyAPI_FUNC
andHPyAPI_STORAGE
were a mess and were not used consistently. It was needed because of the hack in point (3), but now that it's gone we can more easily distinguish theHPy_*
trampolines from the actualctx_*
implementations.HPyAPI_RUNTIME_FUNC
is also no longer needed. NOTE: on master we useHPyAPI_FUNC(int) foo(void);
, but here I chose to useHPyAPI_FUNC int foo(void);
. The reason is that it's much easier to autogenerate, because to emit the former syntax you need to mess withpycparser
nodes.The final result is still not perfect. In particular,
hpy/cpython/misc.h
contains too much stuff, and most of it could be probably autogenerated. However, it can still be done in follow-up PRs.