Timur Tabi
2025-Dec-03 00:42 UTC
[PATCH v2 12/13] gpu: nova-core: add PIO support for loading firmware images
On Tue, 2025-12-02 at 16:35 -0800, John Hubbard wrote:> ??????? for chunk in data.chunks_exact(4) { > ??????????? // PANIC: `chunks_exact(4)` guarantees each chunk is exactly 4 bytes. > ??????????? let word = u32::from_le_bytes(chunk.try_into().unwrap()); > ??????????? regs::NV_PFALCON_FALCON_EMEM_DATA::default() > ??????????????? .set_data(word) > ??????????????? .write(bar, &Fsp::ID); > ??????? } > > ...but actually, I think your way is better, because you don't have > just justify an .unwrap(). > > What do you think?I agree. Avoiding unwrap(), even with a comment, is always a good idea.> I figured you'd enjoy this, coming as it does just one email after I > wrote "never .unwrap()". haha :)I think your code is effectively identical to mine, except that I don't need the PANIC comment. I suspect that in both cases, the compiler cannot tell that each chunk is always 4 bytes and try_into() will never panic. In my case, word[3] always exists and will never panic either. So I'm guess that the compiler will still emit code to check for panic. I don't know.
John Hubbard
2025-Dec-03 00:45 UTC
[PATCH v2 12/13] gpu: nova-core: add PIO support for loading firmware images
On 12/2/25 4:42 PM, Timur Tabi wrote:> On Tue, 2025-12-02 at 16:35 -0800, John Hubbard wrote: >> ??????? for chunk in data.chunks_exact(4) { >> ??????????? // PANIC: `chunks_exact(4)` guarantees each chunk is exactly 4 bytes. >> ??????????? let word = u32::from_le_bytes(chunk.try_into().unwrap()); >> ??????????? regs::NV_PFALCON_FALCON_EMEM_DATA::default() >> ??????????????? .set_data(word) >> ??????????????? .write(bar, &Fsp::ID); >> ??????? } >> >> ...but actually, I think your way is better, because you don't have >> just justify an .unwrap(). >> >> What do you think? > > I agree. Avoiding unwrap(), even with a comment, is always a good idea. > >> I figured you'd enjoy this, coming as it does just one email after I >> wrote "never .unwrap()". haha :) > > I think your code is effectively identical to mine, except that I don't need the PANIC comment. IYes. I'm changing my code over to the non-unwrap approach now. That really is clearly better.> suspect that in both cases, the compiler cannot tell that each chunk is always 4 bytes and > try_into() will never panic. In my case, word[3] always exists and will never panic either. So I'm > guess that the compiler will still emit code to check for panic. I don't know. >Good question. Maybe someone with Rust experience can enlighten us on that one. thanks, -- John Hubbard