John Hubbard
2025-Nov-26 00:31 UTC
[PATCH 09/11] gpu: nova-core: add FalconUCodeDescV2 support
On 11/25/25 3:59 PM, Timur Tabi wrote:> On Mon, 2025-11-17 at 18:10 -0500, Joel Fernandes wrote: >> // First define trait >> pub(crate) trait FalconUCodeDescriptor { >> ??? fn imem_load_size(&self) -> u32; >> ??? fn dmem_load_size(&self) -> u32; >> ??? fn engine_id_mask(&self) -> u16; // V3-only field, V2 returns 0 >> ??? ... >> } > > Isn't it more idiomatic for engine_id_mask() (and any other field that exists only on one version) > to return an Option<u16>?I don't know about idiomatic-ness here, but we have been trying hard to avoid Option in fields. It really makes a mess of things. Other approaches require less special casing, we've found. thanks, -- John Hubbard
Alexandre Courbot
2025-Nov-26 01:05 UTC
[PATCH 09/11] gpu: nova-core: add FalconUCodeDescV2 support
On Wed Nov 26, 2025 at 9:31 AM JST, John Hubbard wrote:> On 11/25/25 3:59 PM, Timur Tabi wrote: >> On Mon, 2025-11-17 at 18:10 -0500, Joel Fernandes wrote: >>> // First define trait >>> pub(crate) trait FalconUCodeDescriptor { >>> ??? fn imem_load_size(&self) -> u32; >>> ??? fn dmem_load_size(&self) -> u32; >>> ??? fn engine_id_mask(&self) -> u16; // V3-only field, V2 returns 0 >>> ??? ... >>> } >> >> Isn't it more idiomatic for engine_id_mask() (and any other field that exists only on one version) >> to return an Option<u16>? > > I don't know about idiomatic-ness here, but we have been trying hard > to avoid Option in fields. It really makes a mess of things. > > Other approaches require less special casing, we've found.IIUC Timur's proposal would not require an Option as a field, just that the method returns one. I tend to agree that this is more idiomatic.