3 ms·

how would the idiomatic code for that example look like? (seriously)

johnisgood 5y ago
Here is the refactored code: https://github.com/immunant/ioq3/blob/refactored/quake3-rs/qagamex86_64/src/qcommon/q_math.rs https://github.com/immunant/ioq3/blob/refactored/quake3-rs/q...
- estebank 5y ago
  I wouldn't call that code idiomatic either: it is cleaned up from the automatic translation, but it still a far cry from what Rust written by hand would look like pub static mut g_color_table: [crate::src::qcommon::q_shared::vec4_t; 8] = [ [0f32, 0f32, 0f32, 1f32], [1f32, 0f32, 0f32, 1f32], [0f32, 1f32, 0f32, 1f32], [1f32, 1f32, 0f32, 1f32], [0f32, 0f32, 1f32, 1f32], [0f32, 1f32, 1f32, 1f32], [1f32, 0f32, 1f32, 1f32], [1f32, 1f32, 1f32, 1f32], ]; would maybe look like pub static mut g_color_table: [[f32; 4]; 8] = [ [0, 0, 0, 1], [1, 0, 0, 1], [0, 1, 0, 1], [1, 1, 0, 1], [0, 0, 1, 1], [0, 1, 1, 1], [1, 0, 1, 1], [1, 1, 1, 1], ]; but it still looks weird to me (a static mut would be uncommon in a design, and not having a new-type to represent these would also be uncommon). If you are interested in looking what Rust written from the ground up in the same domain looks like, you can maybe look at Bevy[1]. The biggest source of "noise" in Rust is explicit type conversions, FP constructs and longer types (`.into()`/`.as_mut_ref()`/`.map()`/`Result<Option<T>, E>`). [1]: https://github.com/bevyengine/bevy/blob/main/crates/bevy_render/src/mesh/mesh.rs https://github.com/bevyengine/bevy/blob/main/crates/bevy_ren...
  - johnisgood 5y ago
    BufferInfo { buffer_usage: BufferUsage::VERTEX, ..Default::default() }, What is "..Default" here?
    - estebank 5y ago
      `Default` is a trait[1] that can be implemented on any type that can be conceived to have a default value. For example, Option has something like: impl<T> Default for Option<T> { fn default() -> Option<T> { None } } If every field of a struct impl Default, then you can you can use the `#[derive(Default)]` annotation to auto-fill that impl for that struct, otherwise you have to write it by hand. The .. is the spread operator, and it takes every field of the value on the right and applies it to every field of the struct constructor on the left. This only works for the same type (it's not only looking at the field names). `Default::default()` is a qualified call on the trait equivalent to `<_ as Default>::default()`, where `_` is inferred from context to be `BufferInfo`. What that code is doing is saying "build a default BufferInfo with field buffer_usage set to VERTEX". The spread operator has a limitation you might have intuited from the description that it can't work for types where some field(s) should always be set but not others. To get around that you can use the builder pattern BufferInfo::with_buffer_usage(BufferUsage::VERTEX).build() and there's a pre-RFC in flight to become an RFC to add support for struct Foo { field: i32 = 42, } let foo = Foo { .. }; [1]: https://doc.rust-lang.org/std/default/trait.Default.html https://doc.rust-lang.org/std/default/trait.Default.html
    - deleted 5y ago
      [deleted]
    - ufmace 5y ago
      Normally in Rust, to initialize a struct, you must specify a value for every field of it, since null and uninitialized values aren't allowed. To make it a little more convenient, there's a trait called Default which can be implemented for your struct to provide an instance with every field set to a value that you think should be the default. And the .. syntax allows you to copy values for the rest of the fields of a struct from another struct instance. So calling that with Default::default() gets an instance with the default values and uses those values to fill all of the rest of the fields of the struct. Thus if you have a big struct where most values are the default most of the time, you can create instances of it with one or two fields set to something different like this without an extra few dozen lines of repetitive boilerplate for every other field of the struct.
- LtdJorge 5y ago
  Bear in mind this is the source code of a game/game engine. They are notorious for their use of hacks, specially to squeeze out performance.
  - johnisgood 5y ago
    In all fairness, the Linux kernel uses various techniques here and there for optimization, too. Where do you see hacks being used in, say, the refactored code? If you meant generally, then sure, they do.
- ggrrhh_ta 5y ago
  Oh, I thought that the comment about not looking as idiomatic code was referring to the kernel::new_device_... sample in a parent/parent HN comment. I was more interested in knowing what was so unidiomatic about that.
  - estebank 5y ago
    The posted code isn't terrible, but it is a bit denser and harder to read than I'd like. Some small changes (like importing the macros to avoid a full path) makes a bit easier to read // SAFETY: This device doesn't support DMA. let base = unsafe { IoMem::try_new(res)? }; let mut data = new_device_data!( RegistrationWithIrqChip::new(), PL061Resources { base, parent_irq: irq }, // SAFETY: We call `spinlock_init` below. unsafe { SpinLock::new(PL061Data::default()) }, "PL061::Registrations" )?; // SAFETY: General part of the data is pinned when `data` is. let gen = unsafe { data.as_mut().map_unchecked_mut(|d| &mut **d) }; spinlock_init!(gen, "PL061::General"); let data: Ref<DeviceData> = data.into(); I imagine code like this would be the body of a small function that does all this unsafe preparations and gives back a Result<Ref<DeviceData>, Error> once it's done, which would change the end to be Ok(data.into())
    - ggrrhh_ta 5y ago
      Thanks, it actually looks better and more understandable.

Here is the refactored code: https://github.com/immunant/ioq3/blob/refactored/quake3-rs/qagamex86_64/src/qcommon/q_math.rs https://github.com/immunant/ioq3/blob/refactored/quake3-rs/q...

estebank 5y ago
I wouldn't call that code idiomatic either: it is cleaned up from the automatic translation, but it still a far cry from what Rust written by hand would look like pub static mut g_color_table: [crate::src::qcommon::q_shared::vec4_t; 8] = [ [0f32, 0f32, 0f32, 1f32], [1f32, 0f32, 0f32, 1f32], [0f32, 1f32, 0f32, 1f32], [1f32, 1f32, 0f32, 1f32], [0f32, 0f32, 1f32, 1f32], [0f32, 1f32, 1f32, 1f32], [1f32, 0f32, 1f32, 1f32], [1f32, 1f32, 1f32, 1f32], ]; would maybe look like pub static mut g_color_table: [[f32; 4]; 8] = [ [0, 0, 0, 1], [1, 0, 0, 1], [0, 1, 0, 1], [1, 1, 0, 1], [0, 0, 1, 1], [0, 1, 1, 1], [1, 0, 1, 1], [1, 1, 1, 1], ]; but it still looks weird to me (a static mut would be uncommon in a design, and not having a new-type to represent these would also be uncommon). If you are interested in looking what Rust written from the ground up in the same domain looks like, you can maybe look at Bevy[1]. The biggest source of "noise" in Rust is explicit type conversions, FP constructs and longer types (`.into()`/`.as_mut_ref()`/`.map()`/`Result<Option<T>, E>`). [1]: https://github.com/bevyengine/bevy/blob/main/crates/bevy_render/src/mesh/mesh.rs https://github.com/bevyengine/bevy/blob/main/crates/bevy_ren...
- johnisgood 5y ago
  BufferInfo { buffer_usage: BufferUsage::VERTEX, ..Default::default() }, What is "..Default" here?
  - estebank 5y ago
    `Default` is a trait[1] that can be implemented on any type that can be conceived to have a default value. For example, Option has something like: impl<T> Default for Option<T> { fn default() -> Option<T> { None } } If every field of a struct impl Default, then you can you can use the `#[derive(Default)]` annotation to auto-fill that impl for that struct, otherwise you have to write it by hand. The .. is the spread operator, and it takes every field of the value on the right and applies it to every field of the struct constructor on the left. This only works for the same type (it's not only looking at the field names). `Default::default()` is a qualified call on the trait equivalent to `<_ as Default>::default()`, where `_` is inferred from context to be `BufferInfo`. What that code is doing is saying "build a default BufferInfo with field buffer_usage set to VERTEX". The spread operator has a limitation you might have intuited from the description that it can't work for types where some field(s) should always be set but not others. To get around that you can use the builder pattern BufferInfo::with_buffer_usage(BufferUsage::VERTEX).build() and there's a pre-RFC in flight to become an RFC to add support for struct Foo { field: i32 = 42, } let foo = Foo { .. }; [1]: https://doc.rust-lang.org/std/default/trait.Default.html https://doc.rust-lang.org/std/default/trait.Default.html
  - deleted 5y ago
    [deleted]
  - ufmace 5y ago
    Normally in Rust, to initialize a struct, you must specify a value for every field of it, since null and uninitialized values aren't allowed. To make it a little more convenient, there's a trait called Default which can be implemented for your struct to provide an instance with every field set to a value that you think should be the default. And the .. syntax allows you to copy values for the rest of the fields of a struct from another struct instance. So calling that with Default::default() gets an instance with the default values and uses those values to fill all of the rest of the fields of the struct. Thus if you have a big struct where most values are the default most of the time, you can create instances of it with one or two fields set to something different like this without an extra few dozen lines of repetitive boilerplate for every other field of the struct.
LtdJorge 5y ago
Bear in mind this is the source code of a game/game engine. They are notorious for their use of hacks, specially to squeeze out performance.
- johnisgood 5y ago
  In all fairness, the Linux kernel uses various techniques here and there for optimization, too. Where do you see hacks being used in, say, the refactored code? If you meant generally, then sure, they do.
ggrrhh_ta 5y ago
Oh, I thought that the comment about not looking as idiomatic code was referring to the kernel::new_device_... sample in a parent/parent HN comment. I was more interested in knowing what was so unidiomatic about that.
- estebank 5y ago
  The posted code isn't terrible, but it is a bit denser and harder to read than I'd like. Some small changes (like importing the macros to avoid a full path) makes a bit easier to read // SAFETY: This device doesn't support DMA. let base = unsafe { IoMem::try_new(res)? }; let mut data = new_device_data!( RegistrationWithIrqChip::new(), PL061Resources { base, parent_irq: irq }, // SAFETY: We call `spinlock_init` below. unsafe { SpinLock::new(PL061Data::default()) }, "PL061::Registrations" )?; // SAFETY: General part of the data is pinned when `data` is. let gen = unsafe { data.as_mut().map_unchecked_mut(|d| &mut **d) }; spinlock_init!(gen, "PL061::General"); let data: Ref<DeviceData> = data.into(); I imagine code like this would be the body of a small function that does all this unsafe preparations and gives back a Result<Ref<DeviceData>, Error> once it's done, which would change the end to be Ok(data.into())
  - ggrrhh_ta 5y ago
    Thanks, it actually looks better and more understandable.