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]> <mailto:[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);
}
]]]
