On Fri, Feb 28, 2025 at 12:08 PM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
>
> On Fri, Feb 28, 2025 at 4:55 PM Tamir Duberstein <tamird@xxxxxxxxx> wrote:
> >
> > On Fri, Feb 28, 2025 at 7:41 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
> > >
> > > @@ -980,8 +983,12 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
> > > /// * If `url_len` > 0, remove the 2 segments header/length and also count the
> > > /// conversion to numeric segments.
> > > /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment.
> > > -#[no_mangle]
> > > -pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
> > > +///
> > > +/// # Safety
> > > +///
> > > +/// Always safe to call.
> >
> > This should explain why it's marked unsafe, since it's always safe to call.
>
> Safety comments generally do not explain rationale for why they are
> the way they are. Where would you like me to put it?
Safety comments also generally do not say that the function isn't
really unsafe (with a notable exception in
`samples/rust/rust_print_main.rs` which is similar to this case).
Perhaps "This function is marked unsafe because ... but since a safety
comment is still required:" would flow nicely into the safety section.
> > > @@ -36,6 +36,10 @@
> > > #include <linux/workqueue.h>
> > > #include <trace/events/rust_sample.h>
> > >
> > > +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> > > +#include <drm/drm_panic.h>
> > > +#endif
> >
> > Why the guard here?
> >
> > It'd be nice to have a comment here explaining the atypical need for
> > this include.
>
> It's not necessary. I can drop it.
Ok. A comment on the include would still be helpful.
