Ash best practices?

[YaLTeR]

I'm experimenting with ash and I find it really nice to work with. However, it's unclear what the best usage patterns are. For example, for structs you can:

• construct them manually

  let desc_alloc_info = vk::DescriptorSetAllocateInfo {
      descriptor_pool: self.pool,
      descriptor_set_count: self.layouts.len() as u32,
      p_set_layouts: self.layouts.as_ptr(),
      ..Default::default()
  };
  

• use builder and call .build()

  let device_create_info = vk::DeviceCreateInfo::builder()
      .queue_create_infos(&queue_info)
      .enabled_extension_names(&device_extension_names_raw)
      .enabled_features(&features)
      .build();
  

• use builder and not call .build()

Another issue is that while I do want ash to take care of the 100%-boilerplate (filling s_type and zero p_next), I do not want to accidentally leave an important field with a default value (which is sometimes not the same as the correct I-don't-care-value). I have already been bitten by this last point once and since then I have to look at the docs to make sure I am aware about every single field. It would be cool if the compiler helped me with this somehow while still allowing to easily set field values as default.

[MaikKlein]

I have to look at the docs to make sure I am aware about every single field. It would be cool if the compiler helped me with this somehow while still allowing to easily set field values as default.

I understand your pain, but I don't think there is an easy way to do this at compile time.

use builder and not call .build()

This is the preferred way. There are a few cases where this is currently not possible and you have to call build yourself.

[YaLTeR]

So I've been calling build() on all of my builders that accept an array:

let create_info = vk::InstanceCreateInfo::builder()
    .application_info(&app_info)
    .enabled_extension_names(&[/* ... */])
    .enabled_layer_names(&[/* ... */])
    .build();

I thought this was the correct way to avoid Rust complaining about temporary values being dropped.

But now I've compiled in release and got a lot of segfaults, presumably because the temporary arrays are indeed being dropped. 😀 I think there needs to be at the very least a BIG WARNING in the build() documentation not to do this.

[Friz64]

There already is a big warning in the rustdoc, it's just easy to miss.

[YaLTeR]

There already is a big warning in the rustdoc, it's just easy to miss.

The only warning I see is "Calling build will discard all the lifetime information." which does not auto-deref to "if you pass in references to temporary arrays your program will explode" in my mind. 😀

[Friz64]

The borrow checker would have told you about your bug with the temporary arrays (because of the lifetime information) i'm pretty sure

[aloucks]

The build() method is effectively unsafe. The compiler cannot enforce it's safety. There has been disagreements around whether or not it should be marked unsafe. My understanding of the unsafe keyword (on functions) is that it must be present when the compiler cannot enforce the invariants that must be upheld for memory safety. There is precedent for this in the standard library. Here is one example. The argument against marking it unsafe was that "it doesn't require any unsafe code to implement".

[Ralith]

Pin::new_unchecked is unsafe because the soundness of other safe interfaces depends upon it, which is not the case here. All Vulkan API calls are unsafe regardless, due the very wide array of statically unchecked soundness requirements. As the Vulkan API calls must consume raw pointers, there is no way to make them safe even in the absence of other concerns. The method you use to come up with your raw pointers does not change this.

[MarijnS95]

Anything left to be done here?

Regarding unsafe, IMO it's a welcome addition to mark any builder's fn build() over a struct containing pointers unsafe to discourage users from calling this. OTOH it's inconvenient when having to pass an array of structs containing pointers where .build() inherently needs to be called, manually checking lifetimes is tedious enough already. The only other hack I could think of is #[deprecated] on fn build() for discouragement, but that's now how deprecation works and as mentioned fn build() is necessary in certain (common) cases :disappointed:

Seems like we can go ahead and close this issue as resolved?