TheDan64 / inkwell

It's a New Kind of Wrapper for Exposing LLVM (Safely)
https://thedan64.github.io/inkwell/
Apache License 2.0
2.38k stars 229 forks source link

`InstructionValue::new` is `pub(crate)` instead of `pub` #424

Open willtunnels opened 1 year ago

willtunnels commented 1 year ago

As of #366, the policy for wrapper types is to be able to (unsafely) convert them to/from their underlying LLVM types. (Aside: I am really glad that this proposal was accepted because I have recently needed some LLVM features that are not even exposed in the LLVM C API yet e.g. GlobalValue::setDSOLocal; upstreaming those things all the way through the LLVM C API to llvm-sys to inkwell will take a long time.)

InstructionValue::new seems to have fallen through the cracks.

EDIT: It seems like there are actually quite a few types that do not have LLVMValueRef to Self conversions. So, the thing to do might be for someone to eventually go through all the core types and make sure they have these conversions in one big pass.

corbanvilla commented 11 months ago

@TheDan64 Yes, this would be very helpful. If I submit a PR to expose the new methods on inkwell::values, would you be willing to merge it?

As per the discussion on #366, I definitely see your point about adding necessary features to inkwell rather than conversion functions. I think there are some cases though where it makes sense to convert, though... For instance, I'm working on an existing inkwell-based compiler and trying to add LLVM Code Coverage instrumentation.

A lot of the LLVM code isn't even exposed in the LLVM C-API, but it's still useful to tie into... In that way, it doesn't really belong in llvm-sys either. However, being able to interact with it through an FFI, and then get back to inkwell typesafety is pretty useful.

A few example interfaces

TheDan64 commented 11 months ago

Yes, of course