Daniel Shahaf wrote on Thu, May 19, 2011 at 13:34:05 +0200:
> 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);
> >
This introduces a build warning:
subversion/libsvn_ra_svn/marshal.c: In function 'read_string':
subversion/libsvn_ra_svn/marshal.c:645: warning: assignment discards qualifiers from pointer target type
(since the RHS is pointer-to-const but the LHS is pointer-to-non-const)
> > 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-25 03:06:54 CEST