stefan2_at_apache.org wrote on Thu, May 19, 2011 at 10:37:06 -0000:
> Author: stefan2
> Date: Thu May 19 10:37:06 2011
> New Revision: 1124677
>
> URL: http://svn.apache.org/viewvc?rev=1124677&view=rev
> Log:
> Introduce a "conversion" method that will extract a svn_string_t
> structure from an existing svn_stringbuf_t instance without the
> need to allocate new memory nor modifying the source structure.
>
> * subversion/include/svn_string.h
> (svn_string_from_stringbuf): declare new API function
> * subversion/libsvn_subr/svn_string.c
> (svn_string_from_stringbuf): implement
> * subversion/libsvn_ra_svn/marshal.c
> (read_string): use the new API instead of a manual cast
>
> Modified:
> subversion/trunk/subversion/include/svn_string.h
> subversion/trunk/subversion/libsvn_ra_svn/marshal.c
> subversion/trunk/subversion/libsvn_subr/svn_string.c
>
> Modified: subversion/trunk/subversion/include/svn_string.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_string.h?rev=1124677&r1=1124676&r2=1124677&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_string.h (original)
> +++ subversion/trunk/subversion/include/svn_string.h Thu May 19 10:37:06 2011
> @@ -177,6 +177,11 @@ svn_string_first_non_whitespace(const sv
> apr_size_t
> svn_string_find_char_backward(const svn_string_t *str, char ch);
>
> +/** Returns the @a svn_string_t information contained in the data and
> + * len members of @a strbuf.
> + */
> +const svn_string_t *
> +svn_string_from_stringbuf(const svn_stringbuf_t *strbuf);
> /** @} */
>
>
>
> Modified: subversion/trunk/subversion/libsvn_ra_svn/marshal.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/marshal.c?rev=1124677&r1=1124676&r2=1124677&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_svn/marshal.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_svn/marshal.c Thu May 19 10:37:06 2011
> @@ -642,7 +642,7 @@ static svn_error_t *read_string(svn_ra_s
> * data and len members in stringbuf.
> */
> item->kind = SVN_RA_SVN_STRING;
> - item->u.string = (svn_string_t *)(&stringbuf->data);
> + item->u.string = svn_string_from_stringbuf(stringbuf);
>
> return SVN_NO_ERROR;
> }
>
> Modified: subversion/trunk/subversion/libsvn_subr/svn_string.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/svn_string.c?rev=1124677&r1=1124676&r2=1124677&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/svn_string.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/svn_string.c Thu May 19 10:37:06 2011
> @@ -235,6 +235,29 @@ svn_string_find_char_backward(const svn_
> return find_char_backward(str->data, str->len, ch);
> }
>
> +const svn_string_t *
> +svn_string_from_stringbuf(const svn_stringbuf_t *strbuf)
> +{
> + /* Both, svn_string_t and svn_stringbuf_t are public API structures
> + * since a couple of releases now. Thus, we can rely on their precise
> + * layout not to change.
> + *
> + * It just so happens that svn_string_t is structurally equivalent
> + * to the (data, len) sub-set of svn_stringbuf_t. There is also no
> + * difference in alignment and padding. So, we can just re-interpret
> + * that part of STRBUF as a svn_string_t.
> + *
> + * However, since svn_string_t does not know about the blocksize
> + * member in svn_stringbuf_t, the returned svn_string_t must not
> + * try to re-allocate its data member. It would possibly be inconsistent
> + * with STRBUF's blocksize member.
Why is this a concern? Are you worried that someone will try to cast
the svn_string_t back to a stringbuf? Or that after calling this API,
someone will attempt to reallocate the stringbuf from its pool?
In any case: I'm not happy about code that leaves such 'links' between
the two structs. I'd suggest to either
* make a deep copy, require a pool
* make a shallow copy, document it as such, and tell callers "Don't do
that" for anything that would corrupt one of the structs.
> + * Hence, the result is a const
> + * structure.
> + *
> + * Modifying the string character content is fine, though.
> + */
> + return (const svn_string_t *)&strbuf->data;
> +}
> +
>
>
> /* svn_stringbuf functions */
>
>
Received on 2011-05-19 13:34:42 CEST