Code review comment for lp:~pkunal-parmar/ubuntu-calendar-app/TimeLineView

Revision history for this message
Olivier Tilloy (osomon) wrote :

> > > Also, this loop is incorrect, as children are destroyed their length
> > > decreases, but the iterator keeps increasing, and will eventually become
> > > invalid. You need to do something like this:
> > >
> > > while (children.length > 0) {
> > > children[0].destroy()
> > > }
> >
> > while (children.length > 0) {
> > children[0].destroy()
> > }
> >
> > This will create a infinite loop, as we are not removing object from
> children
> > array/list. we are just destroying object so length of array will not
> change.
> > so existing loop works fine, but then array length will keep increasing so
> to
> > reset length i am now resetting array with following statement.
> >
> > children = [];
>
> Good point, I should have tested my suggestion first.
> However, I don’t think you should reset the value of children to [], that’s
> likely to break automatic memory management, since we don’t have a guarantee
> of when the children are actually removed from the list. I think the following
> would work (destroying the children in reverse order, thus ensuring the
> iterator never becomes invalid):
>
> function destroyAllChildren() {
> for(var i = children.length; i >= 0; --i) {
> children[i].destroy();
> }
> }

Hmmm, that’s embarassing, I’m struggling with indexes today… Make that:

    function destroyAllChildren() {
        for(var i = children.length - 1; i >= 0; --i) {
            children[i].destroy();
        }
    }

« Back to merge proposal