Nouveau - Mar 2022 - [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

On Mon, 2022-02-28 at 21:56 +0100, Christian K?nig
wrote:
> 
> Am 28.02.22 um 21:42 schrieb James Bottomley:
> > On Mon, 2022-02-28 at 21:07 +0100, Christian K?nig wrote:
> > > Am 28.02.22 um 20:56 schrieb Linus Torvalds:
> > > > On Mon, Feb 28, 2022 at 4:19 AM Christian K?nig
> > > > <christian.koenig at amd.com> wrote:
> > > > [SNIP]
> > > > Anybody have any ideas?
> > > I think we should look at the use cases why code is touching
> > > (pos)
> > > after the loop.
> > > 
> > > Just from skimming over the patches to change this and experience
> > > with the drivers/subsystems I help to maintain I think the
> > > primary pattern looks something like this:
> > > 
> > > list_for_each_entry(entry, head, member) {
> > >       if (some_condition_checking(entry))
> > >           break;
> > > }
> > > do_something_with(entry);
> > 
> > Actually, we usually have a check to see if the loop found
> > anything, but in that case it should something like
> > 
> > if (list_entry_is_head(entry, head, member)) {
> >      return with error;
> > }
> > do_somethin_with(entry);
> > 
> > Suffice?  The list_entry_is_head() macro is designed to cope with
> > the bogus entry on head problem.
> 
> That will work and is also what people already do.
> 
> The key problem is that we let people do the same thing over and
> over again with slightly different implementations.
> 
> Out in the wild I've seen at least using a separate variable, using
> a bool to indicate that something was found and just assuming that
> the list has an entry.
> 
> The last case is bogus and basically what can break badly.
Yes, I understand that.  I'm saying we should replace that bogus checks
of entry->something against some_value loop termination condition with
the list_entry_is_head() macro.  That should be a one line and fairly
mechanical change rather than the explosion of code changes we seem to
have in the patch series.

James

Am 28.02.22 um 22:13 schrieb James Bottomley:
> On Mon, 2022-02-28 at 21:56 +0100, Christian K?nig wrote:
>> Am 28.02.22 um 21:42 schrieb James Bottomley:
>>> On Mon, 2022-02-28 at 21:07 +0100, Christian K?nig wrote:
>>>> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
>>>>> On Mon, Feb 28, 2022 at 4:19 AM Christian K?nig
>>>>> <christian.koenig at amd.com> wrote:
>>>>> [SNIP]
>>>>> Anybody have any ideas?
>>>> I think we should look at the use cases why code is touching
>>>> (pos)
>>>> after the loop.
>>>>
>>>> Just from skimming over the patches to change this and
experience
>>>> with the drivers/subsystems I help to maintain I think the
>>>> primary pattern looks something like this:
>>>>
>>>> list_for_each_entry(entry, head, member) {
>>>>        if (some_condition_checking(entry))
>>>>            break;
>>>> }
>>>> do_something_with(entry);
>>> Actually, we usually have a check to see if the loop found
>>> anything, but in that case it should something like
>>>
>>> if (list_entry_is_head(entry, head, member)) {
>>>       return with error;
>>> }
>>> do_somethin_with(entry);
>>>
>>> Suffice?  The list_entry_is_head() macro is designed to cope with
>>> the bogus entry on head problem.
>> That will work and is also what people already do.
>>
>> The key problem is that we let people do the same thing over and
>> over again with slightly different implementations.
>>
>> Out in the wild I've seen at least using a separate variable, using
>> a bool to indicate that something was found and just assuming that
>> the list has an entry.
>>
>> The last case is bogus and basically what can break badly.
> Yes, I understand that.  I'm saying we should replace that bogus checks
> of entry->something against some_value loop termination condition with
> the list_entry_is_head() macro.  That should be a one line and fairly
> mechanical change rather than the explosion of code changes we seem to
> have in the patch series.
Yes, exactly that's my thinking as well.

Christian.
>
> James
>
>