misc: applied suggestions from code review

crates/mun_codegen/Cargo.toml

@@ -15,7 +15,7 @@ categories = ["Game development", "Mun"]
 [dependencies]
 abi = { version = "=0.2.0", path = "../mun_abi", package = "mun_abi" }
 hir = { version = "=0.2.0", path = "../mun_hir", package = "mun_hir" }
-mun_codegen_macros = { path="../mun_codegen_macros", package="mun_codegen_macros"}
+mun_codegen_macros = { path = "../mun_codegen_macros", package = "mun_codegen_macros" }
 mun_target = { version = "=0.2.0", path = "../mun_target" }
 mun_lld = { version = "=70.2.0", path = "../mun_lld" }
 failure = "0.1.7"

crates/mun_codegen/src/ir/file.rs

@@ -75,7 +75,7 @@ pub(crate) fn ir_query(db: &impl IrDatabase, file_id: FileId) -> Arc<FileIR> {
         ExternalGlobals {
             alloc_handle,
             dispatch_table,
-            type_table: type_table.map(Global::from_raw),
+            type_table: type_table.map(|g| unsafe { Global::from_raw(g) }),
         }
     };
 

crates/mun_codegen/src/ir/type_table.rs

@@ -7,6 +7,7 @@ use hir::{Body, ExprId, InferenceResult};
 use inkwell::module::Linkage;
 use inkwell::{module::Module, targets::TargetData, types::ArrayType, values::PointerValue};
 use std::collections::{BTreeSet, HashMap};
+use std::convert::TryInto;
 use std::ffi::CString;
 use std::{mem, sync::Arc};
 
@@ -25,7 +26,9 @@ impl TypeTable {
     /// Looks for a global symbol with the name of the TypeTable global in the specified `module`.
     /// Returns the global value if it could be found, `None` otherwise.
     pub fn find_global(module: &Module) -> Option<Global<[*const ir::TypeInfo]>> {
-        module.get_global(Self::NAME).map(Global::from_raw)
+        module
+            .get_global(Self::NAME)
+            .map(|g| unsafe { Global::from_raw(g) })
     }
 
     /// Generates a `TypeInfo` lookup through the `TypeTable`, equivalent to something along the
@@ -60,7 +63,7 @@ impl TypeTable {
     pub fn get(module: &Module, type_info: &TypeInfo) -> Option<Global<ir::TypeInfo>> {
         module
             .get_global(&type_info_global_name(type_info))
-            .map(Global::from_raw)
+            .map(|g| unsafe { Global::from_raw(g) })
     }
 
     /// Returns the number of types in the `TypeTable`.
@@ -190,8 +193,16 @@ impl<'a, 'ctx, 'm, D: IrDatabase> TypeTableBuilder<'a, 'ctx, 'm, D> {
                     self.value_context,
                 )
                 .as_value(self.value_context),
-            size_in_bits: type_info.size.bit_size as u32,
-            alignment: type_info.size.alignment as u8,
+            size_in_bits: type_info
+                .size
+                .bit_size
+                .try_into()
+                .expect("could not convert size in bits to smaller size"),
+            alignment: type_info
+                .size
+                .alignment
+                .try_into()
+                .expect("could not convert alignment to smaller size"),
             group: type_info.group.to_abi_type(),
         }
         .as_value(self.value_context);
@@ -210,11 +221,11 @@ impl<'a, 'ctx, 'm, D: IrDatabase> TypeTableBuilder<'a, 'ctx, 'm, D> {
                 let compound_type_ir = (type_info_ir, struct_info_ir).as_value(self.value_context);
                 let compound_global =
                     compound_type_ir.into_const_private_global(&type_ir_name, self.value_context);
-                Global::from_raw(compound_global.value)
+                unsafe { Global::from_raw(compound_global.value) }
             }
         };
 
-        // Insert the value in the case so we dont recompute and generate multiple values.
+        // Insert the value in this case, so we don't recompute and generate multiple values.
         type_info_to_ir.insert(type_info.clone(), global);
 
         global
@@ -278,7 +289,10 @@ impl<'a, 'ctx, 'm, D: IrDatabase> TypeTableBuilder<'a, 'ctx, 'm, D> {
             field_names,
             field_types,
             field_offsets,
-            num_fields: fields.len() as u16,
+            num_fields: fields
+                .len()
+                .try_into()
+                .expect("could not convert num_fields to smaller bit size"),
             memory_kind: hir_struct.data(self.db).memory_kind.clone(),
         }
         .as_value(self.value_context)

crates/mun_codegen/src/ir/types.rs

@@ -1,4 +1,5 @@
 use crate::value::{AsValue, IrValueContext, SizedValueType, TransparentValue, Value};
+use mun_codegen_macros::{AsValue, TestIsAbiCompatible};
 
 impl TransparentValue for abi::Guid {
     type Target = [u8; 16];

crates/mun_codegen/src/ir/types/test.rs

@@ -22,7 +22,7 @@ pub trait TestIsAbiCompatible<T> {
     fn test(abi_value: &T);
 }
 
-/// A trait that is implemented if a type is ABI compatible with with T.
+/// A trait that is implemented if a type is ABI compatible with `T`.
 pub trait IsAbiCompatible<T> {}
 
 /// A trait indicating that a type is *not* compatible. This is a helper trait used to determine
@@ -37,7 +37,7 @@ pub trait IsNotAbiCompatible {
 }
 impl<T> IsNotAbiCompatible for T {}
 
-/// Helper structs that enables extracting the type of a value.
+/// Helper structs that allows extracting the type of a value.
 pub struct AbiTypeHelper<T>(std::marker::PhantomData<T>);
 pub struct AbiAndIrTypeHelper<T, S>(std::marker::PhantomData<T>, std::marker::PhantomData<S>);
 

crates/mun_codegen/src/lib.rs

@@ -4,9 +4,6 @@ mod db;
 #[macro_use]
 mod ir;
 
-#[macro_use]
-extern crate mun_codegen_macros;
-
 #[cfg(test)]
 mod mock;
 #[cfg(test)]

crates/mun_codegen/src/value/global.rs

@@ -14,8 +14,7 @@ use std::marker::PhantomData;
 /// Represents a typed global value. A `Global<T>` can be constructed from any `Value<T>` that can
 /// be converted to a [`inkwell::values::BasicValueEnum`].
 ///
-/// Globals can be used to store data inside an inkwell context which can be referenced to from
-/// code.
+/// Globals can be used to store data inside an inkwell context which can be referenced from code.
 ///
 /// Like `Value<T>` a `Global<T>` is typed on the type of data that it stores.
 pub struct Global<T: ?Sized> {
@@ -35,8 +34,14 @@ impl<T: ?Sized> Clone for Global<T> {
 impl<T: ?Sized> Copy for Global<T> {}
 
 impl<T: ?Sized> Global<T> {
-    /// Creates a `Global<T>` from an underlying value. Use with caution.
-    pub fn from_raw(value: inkwell::values::GlobalValue) -> Self {
+    /// Creates a `Global<T>` from an underlying value.
+    ///
+    /// # Safety
+    ///
+    /// There is no guarantee that the passed value actually represents the type `T`. Sometimes this
+    /// can however be very useful. This method is marked as unsafe since there is also no way to
+    /// check the correctness.
+    pub unsafe fn from_raw(value: inkwell::values::GlobalValue) -> Self {
         Global {
             value,
             data: Default::default(),
@@ -107,8 +112,8 @@ where
     }
 
     /// Converts self into a private const global. A private const global always has Private linkage
-    /// so its only accessible from the module its defined in. Its address is globally insignificant
-    /// because the linker can rename it.
+    /// so its only accessible from the module it's defined in. Its address is globally
+    /// insignificant because the linker can rename it.
     ///
     /// This is useful for constant values that require dynamic sizing like arrays or strings but
     /// still need to be referenced in the code. We can't use const arrays here because the size

crates/mun_codegen/src/value/mod.rs

@@ -28,7 +28,7 @@ use std::hash::Hash;
 /// Represents a generic inkwell value. This is a wrapper around inkwell types that enforces type
 /// safety in the Rust compiler. Rust values that can be converted to inkwell types can be
 /// represented as a value. e.g. `Value<u32>`. Internally this holds an `inkwell::values::IntValue`
-/// but we maintain the information that its actually a `u32` value.
+/// but we maintain the information that it's actually a `u32` value.
 ///
 /// There are several ways to enable a type to be used as a `Value<T>` type through the `AsValue`
 /// trait.
@@ -82,8 +82,8 @@ pub trait AsValue<T: ConcreteValueType + ?Sized> {
     fn as_value(&self, context: &IrValueContext) -> Value<T>;
 }
 
-/// An `TransparentValue` is something that can be represented as a `Value<T>` but which is actually a
-/// rewritten version of another type.
+/// A `TransparentValue` is something that can be represented as a `Value<T>` but which is actually
+/// a rewritten version of another type.
 pub trait TransparentValue {
     type Target: ConcreteValueType + ?Sized;
 
@@ -131,7 +131,7 @@ pub trait PointerValueType: ConcreteValueType {
     ) -> inkwell::types::PointerType;
 }
 
-/// A trait that enables the conversion from an inkwell yype to a corresponding value type. (e.g.
+/// A trait that enables the conversion from an inkwell type to a corresponding value type. (e.g.
 /// IntType -> IntValue)
 pub trait TypeValue {
     type Value: inkwell::values::AnyValue;
@@ -207,7 +207,7 @@ impl<T: ConcreteValueType + ?Sized> Value<T> {
         <T::Value as ValueType>::get_type(&self.value)
     }
 
-    /// Construct a `Value<T>` from an inkwell value
+    /// Constructs a `Value<T>` from an inkwell value.
     pub(super) fn from_raw(value: T::Value) -> Value<T> {
         Value { value }
     }
@@ -285,12 +285,12 @@ where
     }
 }
 
-/// An `TransparentValue` can also be represented by a `Value<T>`
+// A `TransparentValue` can also be represented by a `Value<T>`.
 impl<T: TransparentValue> ConcreteValueType for T {
     type Value = <T::Target as ConcreteValueType>::Value;
 }
 
-/// An `TransparentValue` is sized if the target is also sized
+// A `TransparentValue` is sized if the target is also sized.
 impl<T: TransparentValue> SizedValueType for T
 where
     T::Target: SizedValueType,
@@ -300,7 +300,7 @@ where
     }
 }
 
-/// If the target of the impostor can statically return a pointer type, so can we.
+// If the target of the transparent value can statically return a pointer type, so can we.
 impl<T: TransparentValue> PointerValueType for T
 where
     T::Target: PointerValueType,
@@ -310,7 +310,7 @@ where
     }
 }
 
-/// Impostors can also be represented as a `Value<Self>`
+// Transparent values can also be represented as `Value<Self>`.
 impl<T> AsValue<T> for T
 where
     T: TransparentValue,

crates/mun_codegen/src/value/string.rs

@@ -14,25 +14,29 @@ impl CanInternalize for str {
     type Type = String;
 
     fn intern<S: AsRef<str>>(&self, name: S, context: &IrValueContext) -> Global<Self::Type> {
-        Global::from_raw(
-            self.as_bytes()
-                .as_value(context)
-                .into_const_private_global(name, context)
-                .value,
-        )
+        unsafe {
+            Global::from_raw(
+                self.as_bytes()
+                    .as_value(context)
+                    .into_const_private_global(name, context)
+                    .value,
+            )
+        }
     }
 }
 
 impl CanInternalize for CStr {
     type Type = CString;
 
     fn intern<S: AsRef<str>>(&self, name: S, context: &IrValueContext) -> Global<Self::Type> {
-        Global::from_raw(
-            self.to_bytes_with_nul()
-                .as_value(context)
-                .into_const_private_global(name, context)
-                .value,
-        )
+        unsafe {
+            Global::from_raw(
+                self.to_bytes_with_nul()
+                    .as_value(context)
+                    .into_const_private_global(name, context)
+                    .value,
+            )
+        }
     }
 }
 

crates/mun_codegen_macros/Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 name = "mun_codegen_macros"
 version = "0.1.0"
-authors = ["Bas Zalmstra <[email protected]>"]
+authors = ["The Mun Team <[email protected]>"]
 edition = "2018"
 
 [lib]