Improve different traits about inkwell HOT 10 OPEN

Now that I'm mostly done with the Kaleidoscope example, I think I'm gonna get into this. To me, a single struct should exist with the LLVMValueRef field, and different traits should build on this. That way, common operations such as get_name and set_name is available directly on said struct, and additional methods can be added on top of them.

Furthermore, I guess it could lead to something (not extremely useful, yet possible) like T: FloatValue + PointerValue.

In the end, having to switch from a value to another via as_basic_value, into_float_value, and other methods would extremely quickly get annoying. Any compiler that uses Inkwell would need to cast from different values dynamically all the time (since whether or not it is a FloatValue will be determined dynamically, and not conveniently at compile time like Kaleidoscope), and it would hurt readability.
This loses the safeness of only allowing FloatValue for build_float_add, for example, but it is required, I believe.

from inkwell.

set_name now lives on the BasicValue trait. I tried to moved get_name as well, but figuring out the right lifetime for the return (&'??? CStr) turned out to be a bit tricky, so I punted it for now

from inkwell.

I definitely agree that there should be shared methods across traits and possibly enums, but the tricky part is figuring out to what degree. For example, a method might be applicable to all types represented by a BasicValue/Enum but not all types represented by AnyValue/Enum. There's a lot of work involved in figuring out the differences of what method should apply to which trait. And because this API is still in flux, I've been avoiding figuring that out until it settles down a bit more.

The traits can actually be returned and rust provides us with two ways of doing so: either a heap allocation via Box<Trait> or impl Trait, the latter of which is nightly only. The reason why I decided to make enum equivalents of the traits, and return the enums instead of traits, is because enums have the advantage of pattern matching. So you can pattern match a BasicValueEnum to determine it's an IntValue, but you cannot do this with the BasicValue trait. Applying the Sized restriction to traits does not seem desired, see here for some more info.

from inkwell.

Most of your concerns are fair, indeed. Another problem is that many values can have different types; for example, a PhiValue can also be a float (and thus a FloatValue). Right now I have to use std::mem::transmute to go from one to the other.

from inkwell.

I think it makes sense to provide an as_basic_value() method for PhiValue (or some set smaller than BasicValue if applicable) which provides a BasicValueEnum for use in such a case.

from inkwell.

The thing is that LLVM IR is very strongly typed, but the C API is far less so. With Inkwell, I want to leverage that same strongly type interface from rust (or as close to it as possible). build_float_add should prevent me from inputting pointers, because the LLVM IR will end up complaining anyway at the end of the trail. So I would rather leverage the type system to do so at compile time. I want to write more rust, and less LLVM IR.

I definitely recognize it's not super ergonomic at the moment, and I would like to improve that. The as_ and into_ methods are merely for convenience when you know the type with 100% certainty, the enum should really ideally be matched against.

A real compiler should be storing values as BasicValueEnums, since they are the abstractions so that you don't have to cast from different types. It's true GlobalValue and PhiValue have needed to have casts, but it's only because they're representing multiple concepts at once.

from inkwell.

Indeed, I didn't know the type system was that important, I believe you're right in being more restrictive.

Maybe generics could be used? For example, Phi<FloatValue> would expose FloatValue's members, as well as PhiValue's.

Update: I spent some time thinking everything over, and the best I could come up with is very similar to what's already in place: different structs for FloatValue, IntValue, etc. All these structs implement a trait Value, with get_name, set_name, and most importantly, kind, which returns a ValueEnum, that allows to match the exact kind of the value.

PhiValue could be an associated type, for example. Maybe impl Trait could be used at some point as well (however, it's unstable; but many libraries are).

from inkwell.

I think your idea for Phi<FloatValue> is very close to my plans for sub types in the second iteration of Inkwell (#8).

I think it would make sense to have a trait (name not final) IntoFloatValue which is implemented for purely wrapper or wrapped types (ie FloatValue, GlobalValue<FloatValue>, or PhiValue<FloatValue>)* but not completely different types like PointerValue<FloatValue>. The trait would require something like fn into_float_value(&self) -> FloatValue. That should allow us to mix and match FloatValue, GlobalValue<FloatValue>, and PhiValue<FloatValue> as valid input for build_float_add.

That said, I wanted to keep this to the second iteration, and just focus on getting the foundation working for the first version.

* I oversimplified the signature a bit by excluding the FloatValue subtypes, as those would need to be accounted for as well, but should still be do able.

from inkwell.

By "keep this to the second iteration", do you mean wait till the 0.1.0 milestone is reached? If so, I'll work on it first, since I think this whole thing should be wrapped up as quickly as possible.*

* I think this should be done quickly because these changes will introduce pretty big refactorings, which may impact a previously-written documentation, or some other things.

from inkwell.

Yes, that is what I mean. I'd much rather focus on core functionality such as implementing all (or almost all) llvm methods, multi version compatibility, and documentation before working on further ergonomic and type safety improvements for 0.1.0. Especially since the former will help determine what improvements need to be made.

It's true that some documentation might need to change, but I think the majority of it will be relocating existing methods (along with their docs), so I don't think documentation will be drastically affected (but I could be wrong). We'll probably just have to swap out some identifiers in the docs(&FloatValue -> &IntoFloatValue), and not much else.

from inkwell.