On Thu, Sep 09, 2021 at 09:05:54PM +0300, Lukas Prediger wrote:
> Dear Christoph, Phillip and Randy,
>
> thanks to you all for your comments!
>
Dear Lukas,
You're welcome, thank you for the code.
> >>> Overly long line here, but more importantly this is much cleaner with
> >>> a good old if:
> >>>
> >>>
> >>> if (tmp_info.last_media_change - cdi->last_media_change_ms) < 0)
> >>> tmp_info.has_changed = 1;
> >>>
> >>
> >> Whilst I don't disagree this is technically cleaner, the existing style
> >> certainly read well to me.
>
> The if would additionally require to explicitly initialise .has_changed to
> zero for the else case, so I favored the single assignment that covers
> all cases. I don't have a strong opinion on this, though, so if the if variant
> is generally favored, I can change this. (And I will definitely fix the overlength).
>
Yes true, but I guess your existing style is harder to split across
lines in a clean way. As mentioned, I didn't mind the original code, but
the line length is a fair point. Your call on this one - those with far
more experience than me would probably argue the if/else form though.
> >> In terms of line length, checkpatch doesn't
> >> complain about it, so I guess you mean purely from a visual perspective?
> >
> > Documentation/process/coding-style.rst says:
> >
> > The preferred limit on the length of a single line is 80 columns.
> >
> > checkpatch only checks lines > 100 columns since that is OK in a few
> > cases, like a long quoted string.
> >
> > So try to limit line lengths to 80 columns unless there is some
> > other reason not to do that.
>
> I wasn't aware that checkpatch.pl does not complain if I exceed the 80 cols,
> have fixed those now for an upcoming resubmission.
>
Same, guilty as charged on this one - live and learn I guess :-)
> >>> +{
> >>> + __s64 last_media_change; /* Timestamp of the last detected media
> >>> + * change in ms. May be set by caller, updated
> >>> + * upon successful return of ioctl.
> >>> + */
> >>> + __u64 has_changed; /* Set to 1 by ioctl if last detected media
> >>>
> >>> More overly long lines. Also why is has_changed a u64 if it is used as
> >>> a boolean flag?
> >>
> >> As this is not a packed struct, would not a smaller value still take up
> >> the same space?
> >
> > Might as well be explicit about it and also make it obvious that there
> > is some space available for other fields.
>
> I had this as a __u8 in the first submission but Jens asked me to change it.
> From his feedback on this:
>
> "The struct layout should be modified such that there are no holes or
> padding in it. Probably just make the has_changed a flags thing, and
> make it u64 as well. Then you can define bit 0 to be HAS_CHANGED, and
> that leaves you room to add more flags in the future."
> https://lore.kernel.org/lkml/6d6c533d-465e-33ee-5801-cb7ea84924a8@xxxxxxxxx/
>
Yeah, maybe just a bit more in the comment to emphasize the room for
extra bits in has_changed? I agree it looks fine like this to me though
given the lack of struck packing anyway.
> I changed it to __u64 to address this. We could think about turning it
> back to a __u8 (or bool) and add some explicit padding members
> (a __u8 reserved[3]?), but honestly I don't see much real benefit in that
> compared to how it is now.
>
I agree with you on this personally, I think it's fine.
Regards,
Phil
