You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2011/03/15 05:47:15 UTC
Re: svn commit: r1079686 -
/subversion/trunk/subversion/libsvn_diff/parse-diff.c
stylesen@apache.org wrote on Wed, Mar 09, 2011 at 07:40:38 -0000:
> Author: stylesen
> Date: Wed Mar 9 07:40:38 2011
> New Revision: 1079686
>
> URL: http://svn.apache.org/viewvc?rev=1079686&view=rev
> Log:
> Clean up some deprecated functions.
>
> * subversion/libsvn_diff/parse-diff.c
> (scan_eol, readline): Use svn_io_file_read_full2 which makes finding EOF
> simpler.
>
> Modified:
> subversion/trunk/subversion/libsvn_diff/parse-diff.c
>
> Modified: subversion/trunk/subversion/libsvn_diff/parse-diff.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/parse-diff.c?rev=1079686&r1=1079685&r2=1079686&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_diff/parse-diff.c (original)
> +++ subversion/trunk/subversion/libsvn_diff/parse-diff.c Wed Mar 9 07:40:38 2011
> @@ -287,18 +287,15 @@ scan_eol(const char **eol, apr_file_t *f
> {
> char buf[256];
> apr_size_t len;
> - svn_error_t *err;
> + svn_boolean_t eof = FALSE;
>
You don't need to initialize EOF.
Also, you don't actually *read* that variable anywhere, so you should
drop it entirely :-)
> if (total_len >= max_len)
> break;
>
> len = sizeof(buf) - 1 < (max_len - total_len) ? sizeof(buf) - 1
> : (max_len - total_len);
> - err = svn_io_file_read_full(file, buf, sizeof(buf) - 1, &len, pool);
> - if (err && APR_STATUS_IS_EOF(err->apr_err))
> - svn_error_clear(err);
> - else
> - SVN_ERR(err);
> + SVN_ERR(svn_io_file_read_full2(file, buf, sizeof(buf) - 1, &len, &eof,
> + pool));
sizeof(buf)-1 or sizeof(buf)? The next call passes sizeof(c) (which,
there, is a char).
>
> if (len == 0)
> break; /* EOF */
> @@ -361,14 +358,8 @@ readline(apr_file_t *file,
> len = 0;
> while (*match)
> {
> - svn_error_t *err;
> -
> - err = svn_io_file_read_full(file, &c, sizeof(c), &numbytes,
> - scratch_pool);
> - if (err && APR_STATUS_IS_EOF(err->apr_err))
> - svn_error_clear(err);
> - else
> - SVN_ERR(err);
> + SVN_ERR(svn_io_file_read_full2(file, &c, sizeof(c), &numbytes, eof,
> + scratch_pool));
> len++;
> if (numbytes != 1 || len > max_len)
> {
>
>
Re: svn commit: r1079686 - /subversion/trunk/subversion/libsvn_diff/parse-diff.c
Posted by Senthil Kumaran S <st...@gmail.com>.
Hi Daniel,
On Tue, Mar 15, 2011 at 10:17 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>> Modified: subversion/trunk/subversion/libsvn_diff/parse-diff.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/parse-diff.c?rev=1079686&r1=1079685&r2=1079686&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_diff/parse-diff.c (original)
>> +++ subversion/trunk/subversion/libsvn_diff/parse-diff.c Wed Mar 9 07:40:38 2011
>> @@ -287,18 +287,15 @@ scan_eol(const char **eol, apr_file_t *f
>> {
>> char buf[256];
>> apr_size_t len;
>> - svn_error_t *err;
>> + svn_boolean_t eof = FALSE;
>>
>
> You don't need to initialize EOF.
Done.
> Also, you don't actually *read* that variable anywhere, so you should
> drop it entirely :-)
If I drop this variable and pass a NULL to the new API then I need to
handle "APR_STATUS_IS_EOF" with an 'if...else' as done before. I think
this is more simple and avoids conditional checks.
>> if (total_len >= max_len)
>> break;
>>
>> len = sizeof(buf) - 1 < (max_len - total_len) ? sizeof(buf) - 1
>> : (max_len - total_len);
>> - err = svn_io_file_read_full(file, buf, sizeof(buf) - 1, &len, pool);
>> - if (err && APR_STATUS_IS_EOF(err->apr_err))
>> - svn_error_clear(err);
>> - else
>> - SVN_ERR(err);
>> + SVN_ERR(svn_io_file_read_full2(file, buf, sizeof(buf) - 1, &len, &eof,
>> + pool));
>
> sizeof(buf)-1 or sizeof(buf)? The next call passes sizeof(c) (which,
> there, is a char).
Yes agreed. Done the changes in r1082838.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
http://www.sasenthilkumaran.com/
RE: svn commit: r1079686 - /subversion/trunk/subversion/libsvn_diff/parse-diff.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: dinsdag 15 maart 2011 5:47
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org
> Subject: Re: svn commit: r1079686 -
> /subversion/trunk/subversion/libsvn_diff/parse-diff.c
>
> stylesen@apache.org wrote on Wed, Mar 09, 2011 at 07:40:38 -0000:
> > Author: stylesen
> > Date: Wed Mar 9 07:40:38 2011
> > New Revision: 1079686
> >
> > URL: http://svn.apache.org/viewvc?rev=1079686&view=rev
> > Log:
> > Clean up some deprecated functions.
> >
> > * subversion/libsvn_diff/parse-diff.c
> > (scan_eol, readline): Use svn_io_file_read_full2 which makes finding
EOF
> > simpler.
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_diff/parse-diff.c
> >
> > Modified: subversion/trunk/subversion/libsvn_diff/parse-diff.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/pars
> e-diff.c?rev=1079686&r1=1079685&r2=1079686&view=diff
> >
> ==========================================================
> ====================
> > --- subversion/trunk/subversion/libsvn_diff/parse-diff.c (original)
> > +++ subversion/trunk/subversion/libsvn_diff/parse-diff.c Wed Mar 9
> 07:40:38 2011
> > @@ -287,18 +287,15 @@ scan_eol(const char **eol, apr_file_t *f
> > {
> > char buf[256];
> > apr_size_t len;
> > - svn_error_t *err;
> > + svn_boolean_t eof = FALSE;
> >
>
> You don't need to initialize EOF.
>
> Also, you don't actually *read* that variable anywhere, so you should
> drop it entirely :-)
>
> > if (total_len >= max_len)
> > break;
> >
> > len = sizeof(buf) - 1 < (max_len - total_len) ? sizeof(buf) - 1
> > : (max_len -
total_len);
> > - err = svn_io_file_read_full(file, buf, sizeof(buf) - 1, &len,
pool);
> > - if (err && APR_STATUS_IS_EOF(err->apr_err))
> > - svn_error_clear(err);
> > - else
> > - SVN_ERR(err);
> > + SVN_ERR(svn_io_file_read_full2(file, buf, sizeof(buf) - 1, &len,
&eof,
> > + pool));
>
> sizeof(buf)-1 or sizeof(buf)? The next call passes sizeof(c) (which,
> there, is a char).
This really needs sizeof(buf)-1, to allow adding a final 0 byte after the
read bytes a few lines lower. So we write a 0 outside our allocated memory.
This part of the change breaks the Windows buildbot. Committing a fix in a
few minutes.
Bert