Daniel Shahaf wrote on Thu, May 19, 2011 at 13:34:05 +0200: > 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); > >
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 */ > > > >
