Re: [PATCH 2/2] darray: Evaluate the index parameter only once in darray_remove()
Damien Grassart <[email protected]> Sun, 27 Aug 2017 22:11:14 +0200
| Newsgroups | org.ozlabs.lists.ccan |
|---|---|
| Message-ID | <CADZuF1n3WzKPYSfcwJT7mYHLmwskHKFv78JU6K_qhsRE+sR1tQ@mail.gmail.com> |
--===============5354089590043869722== Content-Type: multipart/alternative; boundary="94eb2c04963aba52bf0557c1c775" --94eb2c04963aba52bf0557c1c775 Content-Type: text/plain; charset="UTF-8" Oops sorry I'll resend with the Signed-off-by. For the underscores, I was trying to keep the same style as the rest of the code, but I can resend a patch with all the "__identifiers" switched to "identifiers__"? On Sun, Aug 27, 2017 at 4:58 AM, David Gibson <[email protected]> wrote: > On Sat, Aug 26, 2017 at 06:26:52PM +0200, Damien Grassart wrote: > > To be consistent with the rest of the code, the index paramater should > > not be evaluated multiple times. Calling this with rand() % arr.size > > would otherwise generally segfault. > > --- > > ccan/darray/darray.h | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > Looks like a definite improvement, but I need a Signed-off-by line in > order to apply it. > > Also.. identifiers starting with _ are technically reserved for system > use. You'd probably get away with it, but it's best to avoid them. I > usually put _ after the name instead for this sort of thing. > > > > > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h > > index 8d47645b..6dd34f08 100644 > > --- a/ccan/darray/darray.h > > +++ b/ccan/darray/darray.h > > @@ -224,8 +224,9 @@ typedef darray(unsigned long) darray_ulong; > > #define darray_pop_check(arr) ((arr).size ? darray_pop(arr) : NULL) > > /* Warning, slow: Requires copying all elements after removed item. */ > > #define darray_remove(arr, index) do { \ > > - if (index < arr.size-1) \ > > - memmove(&(arr).item[index], &(arr).item[index+1], > ((arr).size-1-index)*sizeof(*(arr).item)); \ > > + size_t __index = index; \ > > + if (__index < arr.size-1) \ > > + memmove(&(arr).item[__index], &(arr).item[__index+1], > ((arr).size-1-__index)*sizeof(*(arr).item)); \ > > (arr).size--; \ > > } while(0) > > > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ > _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson > --94eb2c04963aba52bf0557c1c775 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr">Oops sorry I'll resend with the Signed-off-by.<div>For= the underscores, I was trying to keep the same style as the rest of the co= de, but I can resend a patch with all the "__identifiers" switche= d to =C2=A0"identifiers__"?</div></div><div class=3D"gmail_extra"= ><br><div class=3D"gmail_quote">On Sun, Aug 27, 2017 at 4:58 AM, David Gibs= on <span dir=3D"ltr"><<a href=3D"mailto:[email protected]" tar= get=3D"_blank">[email protected]</a>></span> wrote:<br><blockq= uote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc = solid;padding-left:1ex"><span class=3D"">On Sat, Aug 26, 2017 at 06:26:52PM= +0200, Damien Grassart wrote:<br> > To be consistent with the rest of the code, the index paramater should= <br> > not be evaluated multiple times. Calling this with rand() % arr.size<b= r> > would otherwise generally segfault.<br> > ---<br> >=C2=A0 ccan/darray/darray.h | 5 +++--<br> >=C2=A0 1 file changed, 3 insertions(+), 2 deletions(-)<br> <br> </span>Looks like a definite improvement, but I need a Signed-off-by line i= n<br> order to apply it.<br> <br> Also.. identifiers starting with _ are technically reserved for system<br> use.=C2=A0 You'd probably get away with it, but it's best to avoid = them.=C2=A0 I<br> usually put _ after the name instead for this sort of thing.<br> <div class=3D"HOEnZb"><div class=3D"h5"><br> ><br> > diff --git a/ccan/darray/darray.h b/ccan/darray/darray.h<br> > index 8d47645b..6dd34f08 100644<br> > --- a/ccan/darray/darray.h<br> > +++ b/ccan/darray/darray.h<br> > @@ -224,8 +224,9 @@ typedef darray(unsigned long)=C2=A0 darray_ulong;<= br> >=C2=A0 #define darray_pop_check(arr) ((arr).size ? darray_pop(arr) : NU= LL)<br> >=C2=A0 /* Warning, slow: Requires copying all elements after removed it= em. */<br> >=C2=A0 #define darray_remove(arr, index) do { \<br> > -=C2=A0 =C2=A0 =C2=A0if (index < arr.size-1)=C2=A0 =C2=A0 \<br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memmove(&(arr).it= em[index], &(arr).item[index+1], ((arr).size-1-index)*sizeof(*(<wbr>arr= ).item)); \<br> > +=C2=A0 =C2=A0 =C2=A0size_t __index =3D index; \<br> > +=C2=A0 =C2=A0 =C2=A0if (__index < arr.size-1)=C2=A0 =C2=A0 \<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memmove(&(arr).it= em[__index], &(arr).item[__index+1], ((arr).size-1-__index)*sizeof(<wbr= >*(arr).item)); \<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0(arr).size--;=C2=A0 \<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0} while(0)<br> ><br> <br> --<br> </div></div><span class=3D"HOEnZb"><font color=3D"#888888">David Gibson=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | I'= ll have my music baroque, and my code<br> david AT <a href=3D"http://gibson.dropbear.id.au" rel=3D"noreferrer" target= =3D"_blank">gibson.dropbear.id.au</a>=C2=A0 | minimalist, thank you.=C2=A0 = NOT _the_ _other_<br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | _way_ _around_!<br> <a href=3D"http://www.ozlabs.org/~dgibson" rel=3D"noreferrer" target=3D"_bl= ank">http://www.ozlabs.org/~dgibson</a><br> </font></span></blockquote></div><br></div> --94eb2c04963aba52bf0557c1c775-- --===============5354089590043869722== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KY2NhbiBtYWls aW5nIGxpc3QKY2NhbkBsaXN0cy5vemxhYnMub3JnCmh0dHBzOi8vbGlzdHMub3psYWJzLm9yZy9s aXN0aW5mby9jY2FuCg== --===============5354089590043869722==--