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); } ]]] -- Timofei Zhakov
