Messages in this thread | | | Subject | Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop | From | Jakob <> | Date | Wed, 23 Feb 2022 23:08:53 +0100 |
| |
> On 23. Feb 2022, at 21:22, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, Feb 23, 2022 at 12:15 PM Jakob <jakobkoschel@gmail.com> wrote: >> >> in such a case you would still have to set the iterator value to >> NULL when reaching the terminating condition or am I missing something? > > No. > > Make the rule be "you never use the iterator outside the loop". > > IOW, the code sequence is > > some_struct *ptr, *iter;
with C99 iter would be defined within the loop instead right?
> > ptr = NULL; > list_for_each_entry(iter, ...) { > if (iter_matches_condition(iter)) { > ptr = iter; > break; > } > } > > .. never use 'iter' here - you use 'ptr' and check it for NULL .. > > See? Same number of variables as using a separate 'bool found' flag, > but simpler code, and it matches the rule of 'don't use iter outside > the loop'.
ah yes this does make sense. I missed the part of using a separate 'ptr' variable. Thanks for clarifying. I think this is a great idea.
There are cases where pos->member is used (the only legitimate way to use it right now). I suppose those turn into something like this (this example is inspired by dev_add_offload() (net/core/gro.c:38)):
some_struct *ptr, *iter; list_head *list_ptr;
ptr = NULL; list_for_each_entry(iter, head, list) { if (iter_matches_condition(iter)) { ptr = iter; break; } }
if (ptr) list_ptr = head->prev; else list_ptr = iter->list.prev; list_add(..., list_ptr);
before it was simply list_add(..., iter->list.prev);
The other possibility I suppose would be:
if (!ptr) ptr = container_of(head, typeof(*ptr), list) list_add(..., ptr->list.prev);
which leaves you with the same type confusion as before, being far from ideal.
> This is how you'd have to do it anyway if we start using a C99 style > 'declare iter _in_ the loop' model. > > And as mentioned, it actually tends to lead to better code, since the > code outside the loop only has one variable live, not two. > > Of course, compilers can do a lot of optimizations, so a 'found' > variable can be made to generate good code too - if the compiler just > tracks it and notices, and turns the 'break' into a 'goto found', and > the fallthrough into the 'goto not_found'. > > So 'better code generation' is debatable, but even if the compiler can > do as good a job with a separate 'bool' variable and some cleverness, > I think we should strive for code where we make it easy for the > compiler to DTRT - and where the generated code is easier to match up > with what we wrote. > > Linus
If there is interest, I'm happy to send a new patch set once the fixes are clear.
Jakob
| |