You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2011/05/15 20:06:24 UTC
svn commit: r1103490 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Author: stefan2
Date: Sun May 15 18:06:23 2011
New Revision: 1103490
URL: http://svn.apache.org/viewvc?rev=1103490&view=rev
Log:
Eliminate an svn_string_t header allocation and assignment in ra_svn
protocol handling code.
* subversion/libsvn_ra_svn/marshal.c
(read_string): "hero-cast" existing stringbuf into svn_string_t
Modified:
subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1103490&r1=1103489&r2=1103490&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
+++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Sun May 15 18:06:23 2011
@@ -638,11 +638,11 @@ static svn_error_t *read_string(svn_ra_s
}
/* Return the string properly wrapped into an RA_SVN item.
+ * Note that the svn_string_t structure is identical to the
+ * data and len members in stringbuf.
*/
item->kind = SVN_RA_SVN_STRING;
- item->u.string = apr_palloc(pool, sizeof(*item->u.string));
- item->u.string->data = stringbuf->data;
- item->u.string->len = stringbuf->len;
+ item->u.string = (svn_string_t *)(&stringbuf->data);
return SVN_NO_ERROR;
}
Re: svn commit: r1103490 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Posted by Stefan Fuhrmann <eq...@web.de>.
On 20.05.2011 12:38, Philip Martin wrote:
> Stefan Fuhrmann<eq...@web.de> writes:
>
>> On 15.05.2011 20:23, Blair Zajac wrote:
>>> On May 15, 2011, at 11:06 AM, stefan2@apache.org wrote:
>>>> item->kind = SVN_RA_SVN_STRING;
>>>> - item->u.string = apr_palloc(pool, sizeof(*item->u.string));
>>>> - item->u.string->data = stringbuf->data;
>>>> - item->u.string->len = stringbuf->len;
>>>> + item->u.string = (svn_string_t *)(&stringbuf->data);
>>> Is this cast really necessary? I would rather take a small cost hit in preference to the code being safe.
>> r1124677 still does essentially the same but moves
>> it into a separate function and explains in detail how
>> it works and why it is safe.
> It's still not clear, gcc warns:
>
> ../src/subversion/libsvn_ra_svn/marshal.c: In function ‘read_string’:
> ../src/subversion/libsvn_ra_svn/marshal.c:645: warning: assignment discards qualifiers from pointer target type
>
> Your patch says that you are returning a pointer to const because the
> string must not reallocate, but here you are casting away const. Why is
> that safe?
>
Today's series of patches finally addresses that issue and
introduces the new private svn_stringbuf__morph_into_string()
function that does not have the conversion issues anymore.
And it turned out that we can make good use of this function
in quite a number of places.
-- Stefan^2.
Re: svn commit: r1103490 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Fuhrmann <eq...@web.de> writes:
> On 15.05.2011 20:23, Blair Zajac wrote:
>> On May 15, 2011, at 11:06 AM, stefan2@apache.org wrote:
>>> item->kind = SVN_RA_SVN_STRING;
>>> - item->u.string = apr_palloc(pool, sizeof(*item->u.string));
>>> - item->u.string->data = stringbuf->data;
>>> - item->u.string->len = stringbuf->len;
>>> + item->u.string = (svn_string_t *)(&stringbuf->data);
>> Is this cast really necessary? I would rather take a small cost hit in preference to the code being safe.
> r1124677 still does essentially the same but moves
> it into a separate function and explains in detail how
> it works and why it is safe.
It's still not clear, gcc warns:
../src/subversion/libsvn_ra_svn/marshal.c: In function ‘read_string’:
../src/subversion/libsvn_ra_svn/marshal.c:645: warning: assignment discards qualifiers from pointer target type
Your patch says that you are returning a pointer to const because the
string must not reallocate, but here you are casting away const. Why is
that safe?
--
Philip
Re: svn commit: r1103490 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Posted by Stefan Fuhrmann <eq...@web.de>.
On 15.05.2011 20:23, Blair Zajac wrote:
> On May 15, 2011, at 11:06 AM, stefan2@apache.org wrote:
>
>> Author: stefan2
>> Date: Sun May 15 18:06:23 2011
>> New Revision: 1103490
>>
>> URL: http://svn.apache.org/viewvc?rev=1103490&view=rev
>> Log:
>> Eliminate an svn_string_t header allocation and assignment in ra_svn
>> protocol handling code.
>>
>> * subversion/libsvn_ra_svn/marshal.c
>> (read_string): "hero-cast" existing stringbuf into svn_string_t
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_ra_svn/marshal.c
>>
>> Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1103490&r1=1103489&r2=1103490&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
>> +++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Sun May 15 18:06:23 2011
>> @@ -638,11 +638,11 @@ static svn_error_t *read_string(svn_ra_s
>> }
>>
>> /* Return the string properly wrapped into an RA_SVN item.
>> + * Note that the svn_string_t structure is identical to the
>> + * data and len members in stringbuf.
>> */
>> item->kind = SVN_RA_SVN_STRING;
>> - item->u.string = apr_palloc(pool, sizeof(*item->u.string));
>> - item->u.string->data = stringbuf->data;
>> - item->u.string->len = stringbuf->len;
>> + item->u.string = (svn_string_t *)(&stringbuf->data);
> Is this cast really necessary? I would rather take a small cost hit in preference to the code being safe.
r1124677 still does essentially the same but moves
it into a separate function and explains in detail how
it works and why it is safe.
-- Stefan^2.
Re: svn commit: r1103490 - /subversion/trunk/subversion/libsvn_ra_svn/marshal.c
Posted by Blair Zajac <bl...@orcaware.com>.
On May 15, 2011, at 11:06 AM, stefan2@apache.org wrote:
> Author: stefan2
> Date: Sun May 15 18:06:23 2011
> New Revision: 1103490
>
> URL: http://svn.apache.org/viewvc?rev=1103490&view=rev
> Log:
> Eliminate an svn_string_t header allocation and assignment in ra_svn
> protocol handling code.
>
> * subversion/libsvn_ra_svn/marshal.c
> (read_string): "hero-cast" existing stringbuf into svn_string_t
>
> Modified:
> subversion/trunk/subversion/libsvn_ra_svn/marshal.c
>
> Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1103490&r1=1103489&r2=1103490&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Sun May 15 18:06:23 2011
> @@ -638,11 +638,11 @@ static svn_error_t *read_string(svn_ra_s
> }
>
> /* Return the string properly wrapped into an RA_SVN item.
> + * Note that the svn_string_t structure is identical to the
> + * data and len members in stringbuf.
> */
> item->kind = SVN_RA_SVN_STRING;
> - item->u.string = apr_palloc(pool, sizeof(*item->u.string));
> - item->u.string->data = stringbuf->data;
> - item->u.string->len = stringbuf->len;
> + item->u.string = (svn_string_t *)(&stringbuf->data);
Is this cast really necessary? I would rather take a small cost hit in preference to the code being safe.
Blair