-
Notifications
You must be signed in to change notification settings - Fork 95
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
Regenerate LLVMSharp using unsafe bindings. #105
Conversation
I'm fine with holding off on merging this until the LLVMSharp.API namespace can be rewritten. But I don't think I will get to it in the immediate, as I am still focusing on ClangSharp right now. |
I added the |
As the person who added it to start, are you comfortable with the APIs being removed for the time being and being replaced with a lighter-weight wrapper in some future PR? I believe most of the functionality, modulo mirroring the C++ type hierarchy, is exposed in the lower-level wrapper types (e.g. A future PR could then expose the higher-level class wrappers again, but this time calling into the low-level wrappers for the supported functionality. This would essentially give you three levels of use:
|
@tannergooding If @TChatzigiannakis is good with this then we're good, because it hasn't got a lot of traction due to the infrequent (or none I think) releases of LLVMSharp in the intervening time. If @TChatzigiannakis is not unhappy with this, then I'm certainly not going to stop you. |
@tannergooding @mjsabby I'm definitely comfortable with that, and your plan sounds very good to me. Regarding the current
However:
So, from a user perspective, I would like to eventually have the "good" parts of it (either in the C++-like API or in the marshalled API), without the shortcomings that we currently have. (And, of course, let me know if I can help in any way!) |
Thanks @TChatzigiannakis. It's worth noting that for the simple-wrapper, a lot of the "niceties" you wanted will still be supported with the simple wrapper. For example, the simple wrapper types implement
If you wanted to help with redoing the higher level wrapper, I'd be happy to help review/etc. If you look at Namely, you have the raw API in The minimal wrapper uses There is then the "one step higher" wrapper in Given that this final tier is using classes, it also has some logic when creating instances to cache things (so we don't create a new instance every time you want to retrieve a property) and for splitting out methods/properties to only be exposed where they make sense (e.g. getting the |
This is the same as dotnet/ClangSharp#69.
This does, as a part of that, drop support for the
LLVMSharp.API
namespace.I do think that having better bindings (that more closely mirror the LLVM C++ API) is very useful (I did something similar in ClangSharp here: https://github.com/microsoft/ClangSharp/tree/master/sources/ClangSharp.PInvokeGenerator/Types).
However, with the regeneration here, practically the entire
LLVMSharp.API
namespace needs to be rewritten as it doesn't support unsafe code and was duplicating most of the logic that exists in the "simple" wrappers.Ideally, the
LLVMSharp.API
would be rewritten and it would, itself, effectively be a minimal wrapper over the wrapper types (which themselves just abstract away the unsafe code) and it would have a caching layer to avoid a bunch of short lived allocations when traversing the types.