On Mon, Apr 25, 2016 at 04:25:27PM +0200, Bernd Schmidt wrote:
> On 04/25/2016 04:21 PM, Bernd Schmidt wrote:
> >On 04/25/2016 03:30 PM, Trevor Saunders wrote:
> >>On Mon, Apr 25, 2016 at 02:28:51PM +0200, Bernd Schmidt wrote:
> >>>On 04/20/2016 08:22 AM, tbsaunde+...@tbsaunde.org wrote:
> >>>>From: Trevor Saunders <tbsaunde+...@tbsaunde.org>
> >>>
> >>>>+ unsigned int len = cond_list.length ();
> >>>>+ for (unsigned int i = len - 1; i < len; i--)
> >>>
> >>>This is a really icky way to write a loop, the i < len condition
> >>>makes it
> >>>look like a forward one. We have FOR_EACH_VEC_ELT{,_REVERSE}, any
> >>>reason not
> >>>to use these?
> >>
> >>I'll agree that depending on unsigned wrapping is a tad wierd, but
> >>personally I think FOR_EACH_VEC_* are pretty icky, and just forget to
> >>think about them before writing a loop.
> >
> >They're standard inside gcc though, and readability-wise much
> >preferrable to the above IMO.
> >
> >I noticed this pattern in a lot of these patches; at this point I think
> >the best thing to do would be for you to go through all of them, address
> >review comments across the whole set, and then start a new thread with
> >v2 patches of all of them so we can retire this thread.
> >
> Oh, and also - I would prefer for each nontrivial such loop some sort of
> analysis about what order elements were inserted/processed in before this
> change, and what order afterwards. This could be a source of subtle errors
> with this patch series. When removing elements in a loop, we also need to
> pay attention to whether that's still safe.
I didn't write it down, but I did think about then when writing these
patches. Most if not all places push stuff onto the front of a list and
then iterate the list from front to back. Which is the same order as
pushing things on the back of a vector and then iterating the vector
back to front.
Trev
>
>
> Bernd
>
