[prev in list] [next in list] [prev in thread] [next in thread] 

List:       subversion-dev
Subject:    Re: svn commit: r1696626 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
From:       Stefan Fuhrmann <stefan.fuhrmann () wandisco ! com>
Date:       2015-08-21 18:45:04
Message-ID: CA+t0gk09Xt2p5w+_YdaAajLFdMpZAqK4hfX-1CDywQsV1ALSkQ () mail ! gmail ! com
[Download RAW message or body]

On Wed, Aug 19, 2015 at 6:57 PM, Branko Čibej <brane@wandisco.com> wrote:

> On 19.08.2015 18:26, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Wed Aug 19 16:26:05 2015
> > New Revision: 1696626
> > 
> > URL: http://svn.apache.org/r1696626
> > Log:
> > * subversion/libsvn_ra_serf/util.c
> > (ssl_convert_serf_failures): When doing array size calculations, refer
> to
> > the array object only.
> > 
> > Modified:
> > subversion/trunk/subversion/libsvn_ra_serf/util.c
> > 
> > Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1696626&r1=1696625&r2=1696626&view=diff
> 
> > 
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
> > +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Wed Aug 19
> 16:26:05 2015
> > @@ -65,7 +65,9 @@ ssl_convert_serf_failures(int failures)
> > apr_uint32_t svn_failures = 0;
> > apr_size_t i;
> > 
> > -  for (i = 0; i < sizeof(serf_failure_map) / (2 *
> sizeof(apr_uint32_t)); ++i)
> > +  for (i = 0;
> > +       i < sizeof(serf_failure_map) / (sizeof(serf_failure_map[0]));
> > +       ++i)
> > {
> > if (failures & serf_failure_map[i][0])
> > {
> 
> This one would've been better served by introducing a constant for the
> array size ...
> 

I agree and was about to commit the patch. Then I greped
the code and found that we use this pattern quite frequently
(10s of places). There are also 6 local definitions for an
ARRAYLEN or ARRAY_LEN macro that nicely encapsulate
the sizeof() magic.

Given that people seem to like open arrays (spares the
invention of yet another constant), I suggest we introduce
SVN_ARRAY_LEN (or SVN__ARRAY_LEN) and use that
consistently.

-- Stefan^2.


[Attachment #3 (text/html)]

<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 19, 2015 \
at 6:57 PM, Branko Čibej <span dir="ltr">&lt;<a href="mailto:brane@wandisco.com" \
target="_blank">brane@wandisco.com</a>&gt;</span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">On 19.08.2015 18:26, <a \
href="mailto:stefan2@apache.org">stefan2@apache.org</a> wrote:<br> &gt; Author: \
stefan2<br> &gt; Date: Wed Aug 19 16:26:05 2015<br>
&gt; New Revision: 1696626<br>
&gt;<br>
&gt; URL: <a href="http://svn.apache.org/r1696626" rel="noreferrer" \
target="_blank">http://svn.apache.org/r1696626</a><br> &gt; Log:<br>
&gt; * subversion/libsvn_ra_serf/util.c<br>
&gt;     (ssl_convert_serf_failures): When doing array size calculations, refer \
to<br> &gt;                                                the array object only.<br>
&gt;<br>
&gt; Modified:<br>
&gt;        subversion/trunk/subversion/libsvn_ra_serf/util.c<br>
&gt;<br>
&gt; Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c<br>
&gt; URL: <a href="http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1696626&amp;r1=1696625&amp;r2=1696626&amp;view=diff" \
rel="noreferrer" target="_blank">http://svn.apache.org/viewvc/subversion/trunk/subvers \
ion/libsvn_ra_serf/util.c?rev=1696626&amp;r1=1696625&amp;r2=1696626&amp;view=diff</a><br>
 &gt; ==============================================================================<br>
 &gt; --- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)<br>
&gt; +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Wed Aug 19 16:26:05 \
2015<br> &gt; @@ -65,7 +65,9 @@ ssl_convert_serf_failures(int failures)<br>
&gt;      apr_uint32_t svn_failures = 0;<br>
&gt;      apr_size_t i;<br>
&gt;<br>
&gt; -   for (i = 0; i &lt; sizeof(serf_failure_map) / (2 * sizeof(apr_uint32_t)); \
++i)<br> &gt; +   for (i = 0;<br>
&gt; +           i &lt; sizeof(serf_failure_map) / (sizeof(serf_failure_map[0]));<br>
&gt; +           ++i)<br>
&gt;         {<br>
&gt;            if (failures &amp; serf_failure_map[i][0])<br>
&gt;               {<br>
<br>
This one would&#39;ve been better served by introducing a constant for the<br>
array size ...<br></blockquote><br></div><div class="gmail_quote">I agree and was \
about to commit the patch. Then I greped<br>the code and found that we use this \
pattern quite frequently<br></div><div class="gmail_quote">(10s of places). There are \
also 6 local definitions for an<br>ARRAYLEN or ARRAY_LEN macro that nicely \
encapsulate<br>the sizeof() magic.<br><br>Given that people seem to like open arrays \
(spares the<br></div><div class="gmail_quote">invention of yet another constant), I \
suggest we introduce<br></div><div class="gmail_quote">SVN_ARRAY_LEN (or \
SVN__ARRAY_LEN) and use that<br></div><div \
class="gmail_quote">consistently.<br><br></div><div class="gmail_quote">-- \
Stefan^2.<br></div></div></div>



[prev in list] [next in list] [prev in thread] [next in thread]