On Wed, Mar 4, 2026 at 2:59 AM Branko Čibej <[email protected]> wrote:
> On 3. 3. 26 21:13, Timofei Zhakov wrote: > > On Tue, Mar 3, 2026 at 4:51 PM Branko Čibej <[email protected]> wrote: > >> On 3. 3. 26 14:55, Evgeny Kotkov via dev wrote: >> >> Timofei Zhakov <[email protected]> <[email protected]> writes: >> >> >> I prepared a patch that fixes them by adding const where needed. Can anyone >> correct me if I'm misunderstanding something? >> >> I think that the patch indeed improves the situation by propagating the non- >> const types upwards, so that everything happens without implicit const >> discards. So +1 in general. >> >> >> >> I'll note that changes like this one: >> >> --- subversion/libsvn_subr/version.c (revision 1932024) >> +++ subversion/libsvn_subr/version.c (working copy) >> @@ -253,7 +253,7 @@ svn_version__parse_version_string(svn_version_t ** >> require that it be present. */ >> if (pieces->nelts == 3) >> { >> - const char *piece = APR_ARRAY_IDX(pieces, 2, const char *); >> + char *piece = APR_ARRAY_IDX(pieces, 2, char *); >> char *hyphen = strchr(piece, '-'); >> if (hyphen) >> { >> >> >> have much wider consequences than this one-line change. The 'pieces' come >> from 'svn_cstring_split()' which is documented as >> >> /** Divide @a input into substrings, interpreting any char from @a sep >> * as a token separator. >> * >> * Return an array of copies of those substrings (plain const char*), >> * allocating both the array and the copies in @a pool. >> >> >> and this is a public API. So, strictly speaking, the proposed change is >> an API violation, as opposed to what we were doing before with is "just" a >> standard C hack. >> >> If we want to be really sure we're doing everything right then in all the >> cases where we get a 'const char*' from anywhere and then change it, we >> should create a local copy first. I mean, who knows if it isn't pointing to >> a static string in write-protected memory? >> >> Or we could avoid code churn by assuming that whoever is reading the code >> understands C90 semantics. >> >> > There are a few more places where elements of the svn_cstring_split > function are treated as 'char *' instead of 'const char *'. Have a look at > these: > > subversion/libsvn_subr/io.c:3816 > [[[ > /* Tokenize (into our return pool). */ > tokens = svn_cstring_split(buf->data, " \t", TRUE, pool); > if (tokens->nelts < 2) > continue; > > /* The first token in a multi-token line is the media type. > Subsequent tokens are filename extensions associated with > that media type. */ > type = APR_ARRAY_IDX(tokens, 0, const char *); > for (i = 1; i < tokens->nelts; i++) > { > /* We can safely address 'ext' as a non-const string because > * we know svn_cstring_split() allocated it in 'pool' for > us. */ > char *ext = APR_ARRAY_IDX(tokens, i, char *); > fileext_tolower(ext); > svn_hash_sets(types, ext, type); > } > ]]] > > subversion/mod_dav_svn/repos.c:1917 > [[[ > char *keyval = APR_ARRAY_IDX(array, i, char *); > char *equals = strchr(keyval, '='); > if (equals != NULL) > { > *equals = '\0'; > apr_table_set(table, keyval, equals + 1); > } > ]]] > > subversion/svnsync/sync.c:116 > [[[ > char *line = APR_ARRAY_IDX(lines, i, char *); > ... > ]]] > > I would like to suggest explicitly allowing this usage by adjusting the > docstring of svn_cstring_split. It should always be safe to cast 'char *' > to 'const char *' so this doesn't cause any > backward compatibility violation nor changing existing code. > > I think that if all elements are allocated for us in a pool and the caller > "owns" them, they should be able to do whatever they want with those > strings. > > [[[ > Index: subversion/include/svn_string.h > =================================================================== > --- subversion/include/svn_string.h (revision 1932137) > +++ subversion/include/svn_string.h (working copy) > @@ -463,8 +463,8 @@ svn_string_compare_stringbuf(const svn_string_t *s > /** Divide @a input into substrings, interpreting any char from @a sep > * as a token separator. > * > - * Return an array of copies of those substrings (plain const char*), > - * allocating both the array and the copies in @a pool. > + * Return an array of copies of those substrings (plain char*), allocating > + * both the array and the copies in @a pool. > * > * None of the elements added to the array contain any of the > * characters in @a sep_chars, and none of the new elements are empty > Index: subversion/libsvn_subr/string.c > =================================================================== > --- subversion/libsvn_subr/string.c (revision 1932137) > +++ subversion/libsvn_subr/string.c (working copy) > @@ -902,7 +902,7 @@ svn_cstring_split_append(apr_array_header_t *array > } > > if (p[0] != '\0') > - APR_ARRAY_PUSH(array, const char *) = p; > + APR_ARRAY_PUSH(array, char *) = p; > > p = svn_cstring_tokenize(sep_chars, &pats); > } > ]]] > > > > +1, that's a good change, since it's how we've been using the returned > data internally. > Committed in revision r1932154. -- Timofei Zhakov
