You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Timofei Zhakov <ti...@chemodax.net> on 2026/03/02 20:17:20 UTC
Compilation warnings with newer GCC
I'm pretty sure that it's a new feature in the compiler that detects if the
result of a function returning a pointer to a string was saved into
non-constant pointer from const pointer. This seems reasonable and it seems
like consts were misused in those places.
[[[
[39/469] Building C object
CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/gpg_agent.c.o
/home/tima/dev/svn-trunk/subversion/libsvn_subr/gpg_agent.c: In function
‘find_running_gpg_agent’:
/home/tima/dev/svn-trunk/subversion/libsvn_subr/gpg_agent.c:391:6: warning:
assignment discards ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
391 | ep = strchr(p, '\n');
| ^
[85/469] Building C object
CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/subst.c.o
/home/tima/dev/svn-trunk/subversion/libsvn_subr/subst.c: In function
‘build_keywords’:
/home/tima/dev/svn-trunk/subversion/libsvn_subr/subst.c:309:15: warning:
assignment discards ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
309 | sep = strchr(keyword, '=');
| ^
[93/469] Building C object
CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/version.c.o
/home/tima/dev/svn-trunk/subversion/libsvn_subr/version.c: In function
‘svn_version__parse_version_string’:
/home/tima/dev/svn-trunk/subversion/libsvn_subr/version.c:257:22: warning:
initialization discards ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
257 | char *hyphen = strchr(piece, '-');
| ^~~~~~
[144/469] Building C object
CMakeFiles/libsvn_fs_fs.dir/subversion/libsvn_fs_fs/tree.c.o
/home/tima/dev/svn-trunk/subversion/libsvn_fs_fs/tree.c: In function
‘check_newline’:
/home/tima/dev/svn-trunk/subversion/libsvn_fs_fs/tree.c:2488:13: warning:
initialization discards ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
2488 | char *c = strchr(path, '\n');
| ^~~~~~
[190/469] Building C object
CMakeFiles/libsvn_fs.dir/subversion/libsvn_fs/fs-loader.c.o
/home/tima/dev/svn-trunk/subversion/libsvn_fs/fs-loader.c: In function
‘svn_fs__path_valid’:
/home/tima/dev/svn-trunk/subversion/libsvn_fs/fs-loader.c:493:5: warning:
assignment discards ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
493 | c = strchr(path, '\n');
| ^
[226/469] Building C object
CMakeFiles/libsvn_diff.dir/subversion/libsvn_diff/parse-diff.c.o
/home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c: In function
‘git_start’:
/home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c:1582:19:
warning: assignment discards ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
1582 | new_path_marker = strstr(old_path_marker, " b/");
| ^
/home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c:1610:23:
warning: assignment discards ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
1610 | new_path_marker = strstr(new_path_start, " b/");
| ^
]]]
I prepared a patch that fixes them by adding const where needed. Can anyone
correct me if I'm misunderstanding something?
--
Timofei Zhakov
Re: Compilation warnings with newer GCC
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Mon, Mar 2, 2026 at 9:17 PM Timofei Zhakov <ti...@chemodax.net> wrote:
>
> I'm pretty sure that it's a new feature in the compiler that detects if the result of a function returning a pointer to a string was saved into non-constant pointer from const pointer. This seems reasonable and it seems like consts were misused in those places.
>
> [[[
> [39/469] Building C object CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/gpg_agent.c.o
> /home/tima/dev/svn-trunk/subversion/libsvn_subr/gpg_agent.c: In function ‘find_running_gpg_agent’:
> /home/tima/dev/svn-trunk/subversion/libsvn_subr/gpg_agent.c:391:6: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 391 | ep = strchr(p, '\n');
> | ^
> [85/469] Building C object CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/subst.c.o
> /home/tima/dev/svn-trunk/subversion/libsvn_subr/subst.c: In function ‘build_keywords’:
> /home/tima/dev/svn-trunk/subversion/libsvn_subr/subst.c:309:15: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 309 | sep = strchr(keyword, '=');
> | ^
> [93/469] Building C object CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/version.c.o
> /home/tima/dev/svn-trunk/subversion/libsvn_subr/version.c: In function ‘svn_version__parse_version_string’:
> /home/tima/dev/svn-trunk/subversion/libsvn_subr/version.c:257:22: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 257 | char *hyphen = strchr(piece, '-');
> | ^~~~~~
> [144/469] Building C object CMakeFiles/libsvn_fs_fs.dir/subversion/libsvn_fs_fs/tree.c.o
> /home/tima/dev/svn-trunk/subversion/libsvn_fs_fs/tree.c: In function ‘check_newline’:
> /home/tima/dev/svn-trunk/subversion/libsvn_fs_fs/tree.c:2488:13: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 2488 | char *c = strchr(path, '\n');
> | ^~~~~~
> [190/469] Building C object CMakeFiles/libsvn_fs.dir/subversion/libsvn_fs/fs-loader.c.o
> /home/tima/dev/svn-trunk/subversion/libsvn_fs/fs-loader.c: In function ‘svn_fs__path_valid’:
> /home/tima/dev/svn-trunk/subversion/libsvn_fs/fs-loader.c:493:5: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 493 | c = strchr(path, '\n');
> | ^
> [226/469] Building C object CMakeFiles/libsvn_diff.dir/subversion/libsvn_diff/parse-diff.c.o
> /home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c: In function ‘git_start’:
> /home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c:1582:19: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 1582 | new_path_marker = strstr(old_path_marker, " b/");
> | ^
> /home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c:1610:23: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 1610 | new_path_marker = strstr(new_path_start, " b/");
> | ^
> ]]]
>
> I prepared a patch that fixes them by adding const where needed. Can anyone correct me if I'm misunderstanding something?
Committed the patch as r1932158, r1932159, and r1932160. Plus change in
svn_cstring_split in r1932154.
Thanks everyone!
--
Timofei Zhakov
Re: Compilation warnings with newer GCC
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Tue, Mar 3, 2026 at 2:53 PM Evgeny Kotkov via dev <
dev@subversion.apache.org> wrote:
> Branko Čibej <br...@apache.org> writes:
>
> > There is something fishy going on here. For example, this is the
> prototype
> > for strstr:
> >
> > char *strstr(const char*, const char*);
>
> The relevant change here could be the C23 proposal "N3020 - Qualifier-
> preserving Standard Functions", which updated these functions to preserve
> const qualifiers [1, 2]:
>
> QChar *strstr (QChar *s1, const char *s2);
>
> [where QChar stands for "qualified char"]
>
> The prototype `char *strstr (const char*, …)` seems to have been a
> workaround
> for the lack of function overloading in C. But the downside is that it
> does
> not preserve constness and effectively works like an implicit const_cast.
>
I did not know how that works. Thanks for explaining!
>
> > + char *p = NULL;
> > char *ep = NULL;
> > int sd;
> >
> > This makes no sense. Not only is 'p' already const, nothing is being
> > modified through '*p' but something _is_ being modified through '*ep'.
>
> I think the underlying question here and in similar cases is how the
> non-const
> `char* ep` was obtained from `const char *p` in the first place.
>
> In trunk, the const qualifier seems to be silently stripped away when
> passing
> the pointer to functions like strstr() or strchr():
>
> ep = strchr(p, '\n');
>
> Type-wise, this is equivalent to:
>
> ep = (char*)p;
>
> and it's problematic, because it discards the const from the original
> pointer.
>
> To some extent, I would say that using `const char *` is even misleading if
> it's the only pointer available, as it obscures the fact that the data
> eventually get modified without any explicit typecasts:
>
> const char *keyword = APR_ARRAY_IDX(keyword_tokens, i, const char *);
> /* The only pointer we had originally was const. */
>
> if (expand_custom_keywords)
> {
> char *sep;
>
> sep = strchr(keyword, '=');
> if (sep)
> {
> *sep = '\0'; /* But then the data is modified here
> without typecasts */
>
> [1]:
> https://thephd.dev/c23-is-coming-here-is-what-is-on-the-menu#n3020---qualifier-preserving-standard-functions
> [2]: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3020.pdf
>
>
> Thanks,
> Evgeny Kotkov
>
--
Timofei Zhakov
Re: Compilation warnings with newer GCC
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Tue, Mar 3, 2026 at 4:35 PM Branko Čibej <br...@apache.org> wrote:
> On 3. 3. 26 14:53, Evgeny Kotkov wrote:
>
> Branko Čibej <br...@apache.org> <br...@apache.org> writes:
>
>
> There is something fishy going on here. For example, this is the prototype
> for strstr:
>
> char *strstr(const char*, const char*);
>
> The relevant change here could be the C23 proposal "N3020 - Qualifier-
> preserving Standard Functions", which updated these functions to preserve
> const qualifiers [1, 2]:
>
> QChar *strstr (QChar *s1, const char *s2);
>
> [where QChar stands for "qualified char"]
>
> The prototype `char *strstr (const char*, …)` seems to have been a workaround
> for the lack of function overloading in C. But the downside is that it does
> not preserve constness and effectively works like an implicit const_cast.
>
>
> Yes, indeed, but we should be using -std=c90 in our build, and the C90
> stdlib doesn't have those new prototypes. Does the CMake build set the C
> standard parameters?
>
> + char *p = NULL;
> char *ep = NULL;
> int sd;
>
> This makes no sense. Not only is 'p' already const, nothing is being
> modified through '*p' but something _is_ being modified through '*ep'.
>
> I think the underlying question here and in similar cases is how the non-const
> `char* ep` was obtained from `const char *p` in the first place.
>
> In trunk, the const qualifier seems to be silently stripped away when passing
> the pointer to functions like strstr() or strchr():
>
> ep = strchr(p, '\n');
>
> Type-wise, this is equivalent to:
>
> ep = (char*)p;
>
> and it's problematic, because it discards the const from the original pointer.
>
>
> Right. This is what we old-timer C programmers call a "horrible hack but
> it works" :)
>
> To some extent, I would say that using `const char *` is even misleading if
> it's the only pointer available, as it obscures the fact that the data
> eventually get modified without any explicit typecasts:
>
> const char *keyword = APR_ARRAY_IDX(keyword_tokens, i, const char *);
> /* The only pointer we had originally was const. */
>
> if (expand_custom_keywords)
> {
> char *sep;
>
> sep = strchr(keyword, '=');
> if (sep)
> {
> *sep = '\0'; /* But then the data is modified here
> without typecasts */
>
>
> This is how the C standard library (used to) work ... a compromise. It's
> confusing and misleading and causes way too many bugs and is often abused
> for precisely this reason, to avoid explicit casts. That's one of the
> reasons why we force -std=c90.
>
> -- Brane
>
>
In fact, this warning also occurs with an explicit --std=c90. The QChar
thing is just a nice way to ensure that there are no implicit const_casts
from functions like strchr.
This might reveal real issues just like when a 'const char *' is explicitly
casted into a 'char *'. If an argument is given to a function and marked as
const, it assumes that the function does not change it.
By the way, it is true that CMake does not properly set up the C standard.
--
Timofei Zhakov
Re: Compilation warnings with newer GCC
Posted by Branko Čibej <br...@apache.org>.
On 3. 3. 26 14:53, Evgeny Kotkov wrote:
> Branko Čibej<br...@apache.org> writes:
>
>> There is something fishy going on here. For example, this is the prototype
>> for strstr:
>>
>> char *strstr(const char*, const char*);
> The relevant change here could be the C23 proposal "N3020 - Qualifier-
> preserving Standard Functions", which updated these functions to preserve
> const qualifiers [1, 2]:
>
> QChar *strstr (QChar *s1, const char *s2);
>
> [where QChar stands for "qualified char"]
>
> The prototype `char *strstr (const char*, …)` seems to have been a workaround
> for the lack of function overloading in C. But the downside is that it does
> not preserve constness and effectively works like an implicit const_cast.
Yes, indeed, but we should be using -std=c90 in our build, and the C90
stdlib doesn't have those new prototypes. Does the CMake build set the C
standard parameters?
>> + char *p = NULL;
>> char *ep = NULL;
>> int sd;
>>
>> This makes no sense. Not only is 'p' already const, nothing is being
>> modified through '*p' but something _is_ being modified through '*ep'.
> I think the underlying question here and in similar cases is how the non-const
> `char* ep` was obtained from `const char *p` in the first place.
>
> In trunk, the const qualifier seems to be silently stripped away when passing
> the pointer to functions like strstr() or strchr():
>
> ep = strchr(p, '\n');
>
> Type-wise, this is equivalent to:
>
> ep = (char*)p;
>
> and it's problematic, because it discards the const from the original pointer.
Right. This is what we old-timer C programmers call a "horrible hack but
it works" :)
> To some extent, I would say that using `const char *` is even misleading if
> it's the only pointer available, as it obscures the fact that the data
> eventually get modified without any explicit typecasts:
>
> const char *keyword = APR_ARRAY_IDX(keyword_tokens, i, const char *);
> /* The only pointer we had originally was const. */
>
> if (expand_custom_keywords)
> {
> char *sep;
>
> sep = strchr(keyword, '=');
> if (sep)
> {
> *sep = '\0'; /* But then the data is modified here
> without typecasts */
This is how the C standard library (used to) work ... a compromise. It's
confusing and misleading and causes way too many bugs and is often
abused for precisely this reason, to avoid explicit casts. That's one of
the reasons why we force -std=c90.
-- Brane
Re: Compilation warnings with newer GCC
Posted by Evgeny Kotkov via dev <de...@subversion.apache.org>.
Branko Čibej <br...@apache.org> writes:
> There is something fishy going on here. For example, this is the prototype
> for strstr:
>
> char *strstr(const char*, const char*);
The relevant change here could be the C23 proposal "N3020 - Qualifier-
preserving Standard Functions", which updated these functions to preserve
const qualifiers [1, 2]:
QChar *strstr (QChar *s1, const char *s2);
[where QChar stands for "qualified char"]
The prototype `char *strstr (const char*, …)` seems to have been a workaround
for the lack of function overloading in C. But the downside is that it does
not preserve constness and effectively works like an implicit const_cast.
> + char *p = NULL;
> char *ep = NULL;
> int sd;
>
> This makes no sense. Not only is 'p' already const, nothing is being
> modified through '*p' but something _is_ being modified through '*ep'.
I think the underlying question here and in similar cases is how the non-const
`char* ep` was obtained from `const char *p` in the first place.
In trunk, the const qualifier seems to be silently stripped away when passing
the pointer to functions like strstr() or strchr():
ep = strchr(p, '\n');
Type-wise, this is equivalent to:
ep = (char*)p;
and it's problematic, because it discards the const from the original pointer.
To some extent, I would say that using `const char *` is even misleading if
it's the only pointer available, as it obscures the fact that the data
eventually get modified without any explicit typecasts:
const char *keyword = APR_ARRAY_IDX(keyword_tokens, i, const char *);
/* The only pointer we had originally was const. */
if (expand_custom_keywords)
{
char *sep;
sep = strchr(keyword, '=');
if (sep)
{
*sep = '\0'; /* But then the data is modified here
without typecasts */
[1]: https://thephd.dev/c23-is-coming-here-is-what-is-on-the-menu#n3020---qualifier-preserving-standard-functions
[2]: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3020.pdf
Thanks,
Evgeny Kotkov
Re: Compilation warnings with newer GCC
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Mon, Mar 2, 2026 at 11:34 PM Branko Čibej <br...@apache.org> wrote:
> On 2. 3. 26 21:17, Timofei Zhakov wrote:
>
> I'm pretty sure that it's a new feature in the compiler that detects if
> the result of a function returning a pointer to a string was saved into
> non-constant pointer from const pointer. This seems reasonable and it seems
> like consts were misused in those places.
>
> [[[
> [39/469] Building C object
> CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/gpg_agent.c.o
> /home/tima/dev/svn-trunk/subversion/libsvn_subr/gpg_agent.c: In function
> ‘find_running_gpg_agent’:
> /home/tima/dev/svn-trunk/subversion/libsvn_subr/gpg_agent.c:391:6:
> warning: assignment discards ‘const’ qualifier from pointer target type
> [-Wdiscarded-qualifiers]
> 391 | ep = strchr(p, '\n');
>
>
> But then in your patch:
>
> --- subversion/libsvn_subr/gpg_agent.c (revision 1932024)
> +++ subversion/libsvn_subr/gpg_agent.c (working copy)
> @@ -326,7 +326,7 @@ find_running_gpg_agent(int *new_sd, apr_pool_t *po
> char *buffer;
> const char *socket_name = find_gpg_agent_socket(pool, pool);
> const char *request = NULL;
> - const char *p = NULL;
> + char *p = NULL;
> char *ep = NULL;
> int sd;
>
>
> This makes no sense. Not only is 'p' already const, nothing is being
> modified through '*p' but something _is_ being modified through '*ep'.
>
Later in the code:
[[[
ep = strchr(p, '\n');
if (ep != NULL)
*ep = '\0';
]]]
'ep' points to the same memory as 'p' since strchr essentially takes the
pointer and moves it. So for it to modify the string (assgning *ep to \0),
ep needs to be mutable hence does p.
>
>
> [226/469] Building C object
> CMakeFiles/libsvn_diff.dir/subversion/libsvn_diff/parse-diff.c.o
> /home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c: In function
> ‘git_start’:
> /home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c:1582:19:
> warning: assignment discards ‘const’ qualifier from pointer target type
> [-Wdiscarded-qualifiers]
> 1582 | new_path_marker = strstr(old_path_marker, " b/");
> | ^
> /home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c:1610:23:
> warning: assignment discards ‘const’ qualifier from pointer target type
> [-Wdiscarded-qualifiers]
> 1610 | new_path_marker = strstr(new_path_start, " b/");
> | ^
> ]]]
>
>
> Same here, your patch does this:
>
> --- subversion/libsvn_diff/parse-diff.c (revision 1932024)
> +++ subversion/libsvn_diff/parse-diff.c (working copy)
> @@ -1545,12 +1545,12 @@ static svn_error_t *
> git_start(enum parse_state *new_state, char *line, svn_patch_t *patch,
> apr_pool_t *result_pool, apr_pool_t *scratch_pool)
> {
> - const char *old_path_start;
> + char *old_path_start;
> char *old_path_end;
> - const char *new_path_start;
> - const char *new_path_end;
> + char *new_path_start;
> + char *new_path_end;
> char *new_path_marker;
> - const char *old_path_marker;
> + char *old_path_marker;
>
>
> Again, removing the 'const's makes no sense.
>
>
Here mutability needs to be promoted by all of these pointers to the memory
being modified. Actually I did a double check and new_path_end can remain
constant. The only mutation happens here:
[[[
/* Are the paths before and after the " b/" marker the same? */
if (len_old == len_new
&& ! strncmp(old_path_start, new_path_start, len_old))
{
*old_path_end = '\0';
SVN_ERR(grab_filename(&patch->old_filename, old_path_start,
result_pool, scratch_pool));
SVN_ERR(grab_filename(&patch->new_filename, new_path_start,
result_pool, scratch_pool));
break;
}
]]]
> I prepared a patch that fixes them by adding const where needed. Can
> anyone correct me if I'm misunderstanding something?
>
>
>
> There is something fishy going on here. For example, this is the prototype
> for strstr:
>
> char *strstr(const char*, const char*);
>
>
> and if you look at this bit of code in your error report:
>
> new_path_marker = strstr(new_path_start, " b/");
>
>
> new_path_start is a 'const char*', " b/" is a 'char*' (in C, but a 'const
> char*' in C++), and new_path_marker is a 'const char*'. In the case of both
> the second parameter and the return value, we're assigning a 'char*' to a
> 'const char*', this conversion is implicit and safe and there should be no
> error reported here.
>
> The warning message makes no sense at all.
>
> -- Brane
>
>
--
Timofei Zhakov
Re: Compilation warnings with newer GCC
Posted by Branko Čibej <br...@apache.org>.
On 2. 3. 26 21:17, Timofei Zhakov wrote:
> I'm pretty sure that it's a new feature in the compiler that detects
> if the result of a function returning a pointer to a string was saved
> into non-constant pointer from const pointer. This seems reasonable
> and it seems like consts were misused in those places.
>
> [[[
> [39/469] Building C object
> CMakeFiles/libsvn_subr.dir/subversion/libsvn_subr/gpg_agent.c.o
> /home/tima/dev/svn-trunk/subversion/libsvn_subr/gpg_agent.c: In
> function ‘find_running_gpg_agent’:
> /home/tima/dev/svn-trunk/subversion/libsvn_subr/gpg_agent.c:391:6:
> warning: assignment discards ‘const’ qualifier from pointer target
> type [-Wdiscarded-qualifiers]
> 391 | ep = strchr(p, '\n');
But then in your patch:
--- subversion/libsvn_subr/gpg_agent.c (revision 1932024)
+++ subversion/libsvn_subr/gpg_agent.c (working copy)
@@ -326,7 +326,7 @@ find_running_gpg_agent(int *new_sd, apr_pool_t *po
char *buffer;
const char *socket_name = find_gpg_agent_socket(pool, pool);
const char *request = NULL;
- const char *p = NULL;
+ char *p = NULL;
char *ep = NULL;
int sd;
This makes no sense. Not only is 'p' already const, nothing is being
modified through '*p' but something _is_ being modified through '*ep'.
> [226/469] Building C object
> CMakeFiles/libsvn_diff.dir/subversion/libsvn_diff/parse-diff.c.o
> /home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c: In
> function ‘git_start’:
> /home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c:1582:19:
> warning: assignment discards ‘const’ qualifier from pointer target
> type [-Wdiscarded-qualifiers]
> 1582 | new_path_marker = strstr(old_path_marker, " b/");
> | ^
> /home/tima/dev/svn-trunk/subversion/libsvn_diff/parse-diff.c:1610:23:
> warning: assignment discards ‘const’ qualifier from pointer target
> type [-Wdiscarded-qualifiers]
> 1610 | new_path_marker = strstr(new_path_start, " b/");
> | ^
> ]]]
Same here, your patch does this:
--- subversion/libsvn_diff/parse-diff.c (revision 1932024)
+++ subversion/libsvn_diff/parse-diff.c (working copy)
@@ -1545,12 +1545,12 @@ static svn_error_t *
git_start(enum parse_state *new_state, char *line, svn_patch_t *patch,
apr_pool_t *result_pool, apr_pool_t *scratch_pool)
{
- const char *old_path_start;
+ char *old_path_start;
char *old_path_end;
- const char *new_path_start;
- const char *new_path_end;
+ char *new_path_start;
+ char *new_path_end;
char *new_path_marker;
- const char *old_path_marker;
+ char *old_path_marker;
Again, removing the 'const's makes no sense.
> I prepared a patch that fixes them by adding const where needed. Can
> anyone correct me if I'm misunderstanding something?
There is something fishy going on here. For example, this is the
prototype for strstr:
char *strstr(const char*, const char*);
and if you look at this bit of code in your error report:
new_path_marker = strstr(new_path_start, " b/");
new_path_start is a 'const char*', " b/" is a 'char*' (in C, but a
'const char*' in C++), and new_path_marker is a 'const char*'. In the
case of both the second parameter and the return value, we're assigning
a 'char*' to a 'const char*', this conversion is implicit and safe and
there should be no error reported here.
The warning message makes no sense at all.
-- Brane
Re: Compilation warnings with newer GCC
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Tue, Mar 3, 2026 at 2:55 PM Evgeny Kotkov <ev...@axiorema.com>
wrote:
> Timofei Zhakov <ti...@chemodax.net> 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.
>
> One suggestion that I have is that it might be better to separate the
> simpler
> cases (like those in libsvn_fs/fs-loader.c:svn_fs__path_valid() and
> libsvn_fs_fs/tree.c:check_newline()) into their own patch or commit,
> keeping them distinct from the more complex cases.
>
+1, to separate those changes.
--
Timofei Zhakov
Re: Compilation warnings with newer GCC
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Wed, Mar 4, 2026 at 2:59 AM Branko Čibej <br...@apache.org> wrote:
> On 3. 3. 26 21:13, Timofei Zhakov wrote:
>
> On Tue, Mar 3, 2026 at 4:51 PM Branko Čibej <br...@apache.org> wrote:
>
>> On 3. 3. 26 14:55, Evgeny Kotkov via dev wrote:
>>
>> Timofei Zhakov <ti...@chemodax.net> <ti...@chemodax.net> 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
Re: Compilation warnings with newer GCC
Posted by Branko Čibej <br...@apache.org>.
On 3. 3. 26 21:13, Timofei Zhakov wrote:
> On Tue, Mar 3, 2026 at 4:51 PM Branko Čibej <br...@apache.org> wrote:
>
> On 3. 3. 26 14:55, Evgeny Kotkov via dev wrote:
>> Timofei Zhakov<ti...@chemodax.net> <ma...@chemodax.net> 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.
-- Brane
Re: Compilation warnings with newer GCC
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Tue, Mar 3, 2026 at 4:51 PM Branko Čibej <br...@apache.org> wrote:
> On 3. 3. 26 14:55, Evgeny Kotkov via dev wrote:
>
> Timofei Zhakov <ti...@chemodax.net> <ti...@chemodax.net> 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
Re: Compilation warnings with newer GCC
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Tue, Mar 3, 2026 at 4:51 PM Branko Čibej <br...@apache.org> wrote:
> On 3. 3. 26 14:55, Evgeny Kotkov via dev wrote:
>
> Timofei Zhakov <ti...@chemodax.net> <ti...@chemodax.net> 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.
>
It is not a hack because the docstring explicitly states that the result
would contain "copies" of those substrings. Which technically means that we
are free to do whatever we want with this memory.
I think in this exact example the docstring of svn_cstring_split itself is
slightly misleading, because the elements in the result array can be
treated as 'char *' as well.
>
> 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.
>
If a string is received as an argument of a function and is marked as
'const char *', it could be in write-protected memory which libsvn should
be able to deal with. It should be safe for users to assume that it never
changes. That is why the const modifier exists in the first place. It gives
some guarantees about the API.
--
Timofei Zhakov
Re: Compilation warnings with newer GCC
Posted by Branko Čibej <br...@apache.org>.
On 3. 3. 26 14:55, Evgeny Kotkov via dev wrote:
> Timofei Zhakov<ti...@chemodax.net> 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.
-- Brane
Re: Compilation warnings with newer GCC
Posted by Evgeny Kotkov via dev <de...@subversion.apache.org>.
Timofei Zhakov <ti...@chemodax.net> 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.
One suggestion that I have is that it might be better to separate the simpler
cases (like those in libsvn_fs/fs-loader.c:svn_fs__path_valid() and
libsvn_fs_fs/tree.c:check_newline()) into their own patch or commit,
keeping them distinct from the more complex cases.
Thanks,
Evgeny Kotkov