stef...@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 */ > >
