Re: [PATCH] iio: imu: adis16550: rework clock range test

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Jul 02, 2025 at 10:07:17AM -0500, David Lechner wrote:
> On 7/2/25 9:59 AM, Andy Shevchenko wrote:
> > On Wed, Jul 02, 2025 at 05:53:57PM +0300, Andy Shevchenko wrote:
> >> On Wed, Jul 02, 2025 at 09:27:45AM -0500, David Lechner wrote:
> >>> Rework the clock rate range test to test if sync_mode_data != NULL
> >>> instead of testing if the for loop index variable. This makes it easier
> >>> for static analyzers to see that we aren't using an uninitialized
> >>> sync_mode_data [1].
> >>
> >> But at the same time it makes it not to be the usual pattern.,,
> > 
> > Reading the static analyser output I think the first hunk is only what we need,
> > but this is still false positive and it's problem of that static
> > analyser. Have you filed a bug there? (My point is that modifying the code for
> > the advantage of false positives of some static analyser is wrong road to go
> > in my opinion.)
> 
> I agree that we shouldn't fix this _only_ to make the static analyzer
> happy. But I had to think quite a bit harder to see that the existing
> code was correct compared to what I have proposed here.
> 
> But if this is a common pattern that I just haven't learned to identify
> at a glance yet and everybody else can easily see that the existing code
> is correct, then perhaps it isn't worth the change.

To me checking against index variable (when it's integer, obviously) is correct
thing to do and regular pattern. OTOH, if the "index" is a pointer and rather
we call it "iterator", the angle of view is different because in some cases
it may lead to stale or invalid value which might be mistakenly dereferenced or
speculated (see more in the discussion about list entry APIs [entry is a
keyword here] and if list_entry_is_head() is a good approach.)

-- 
With Best Regards,
Andy Shevchenko