You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by iv...@apache.org on 2014/09/19 17:21:15 UTC
svn commit: r1626247 - /subversion/trunk/subversion/libsvn_ra_svn/client.c
Author: ivan
Date: Fri Sep 19 15:21:15 2014
New Revision: 1626247
URL: http://svn.apache.org/r1626247
Log:
Resolve another compiler warning.
* subversion/libsvn_ra_svn/client.c
(find_tunnel_agent): Add non-const local variable ARGV to fix compiler
warning.
Modified:
subversion/trunk/subversion/libsvn_ra_svn/client.c
Modified: subversion/trunk/subversion/libsvn_ra_svn/client.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/client.c?rev=1626247&r1=1626246&r2=1626247&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_svn/client.c (original)
+++ subversion/trunk/subversion/libsvn_ra_svn/client.c Fri Sep 19 15:21:15 2014
@@ -368,15 +368,16 @@ ra_svn_get_reporter(svn_ra_svn__session_
/* --- RA LAYER IMPLEMENTATION --- */
-/* (Note: *ARGV is an output parameter.) */
+/* (Note: *ARGV_P is an output parameter.) */
static svn_error_t *find_tunnel_agent(const char *tunnel,
const char *hostinfo,
- const char ***argv,
+ const char ***argv_p,
apr_hash_t *config, apr_pool_t *pool)
{
svn_config_t *cfg;
const char *val, *var, *cmd;
char **cmd_argv;
+ const char **argv;
apr_size_t len;
apr_status_t status;
int n;
@@ -430,16 +431,22 @@ static svn_error_t *find_tunnel_agent(co
if (status != APR_SUCCESS)
return svn_error_wrap_apr(status, _("Can't tokenize command '%s'"), cmd);
- /* Append the fixed arguments to the result. */
+ /* Calc number of the fixed arguments. */
for (n = 0; cmd_argv[n] != NULL; n++)
;
- *argv = apr_palloc(pool, (n + 4) * sizeof(char *));
- memcpy(*argv, cmd_argv, n * sizeof(char *));
- (*argv)[n++] = svn_path_uri_decode(hostinfo, pool);
- (*argv)[n++] = "svnserve";
- (*argv)[n++] = "-t";
- (*argv)[n] = NULL;
+ argv = apr_palloc(pool, (n + 4) * sizeof(char *));
+
+ /* Append the fixed arguments to the result. */
+ for (n = 0; cmd_argv[n] != NULL; n++)
+ argv[n] = cmd_argv[n];
+
+ argv[n++] = svn_path_uri_decode(hostinfo, pool);
+ argv[n++] = "svnserve";
+ argv[n++] = "-t";
+ argv[n] = NULL;
+
+ *argv_p = argv;
return SVN_NO_ERROR;
}
Re: svn commit: r1626247 - /subversion/trunk/subversion/libsvn_ra_svn/client.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 20 September 2014 12:11, Branko Čibej <br...@wandisco.com> wrote:
> On 19.09.2014 17:21, ivan@apache.org wrote:
>
> Author: ivan
> Date: Fri Sep 19 15:21:15 2014
> New Revision: 1626247
>
> URL: http://svn.apache.org/r1626247
> Log:
> Resolve another compiler warning.
>
> * subversion/libsvn_ra_svn/client.c
> (find_tunnel_agent): Add non-const local variable ARGV to fix compiler
> warning.
>
>
>
Hi Brane,
> Which compiler is that, and what warning does it emit? There is nothing
> wrong with the code, and I've not seen any warnings from that code in ages,
> with either gcc or clang. Specifically:
>
I was getting warning on client.c:437 (warning C4090: 'function':
different 'const' qualifiers) when compiling using VS2010. The line
437 is memcpy() call:
[[
memcpy(*argv, cmd_argv, n * sizeof(char *));
]]
>
> -/* (Note: *ARGV is an output parameter.) */
> +/* (Note: *ARGV_P is an output parameter.) */
> static svn_error_t *find_tunnel_agent(const char *tunnel,
> const char *hostinfo,
> - const char ***argv,
> + const char ***argv_p,
> apr_hash_t *config, apr_pool_t *pool)
>
>
> The original 'argv' parameter is not constant. It is a non-const pointer to
> a non-const array of (const char*). There should be no warnings when
> assigning to the pointer, and no warnings when modifying the array. The only
> thing you can't do is modify the data the array elements are pointing to,
> but the code did not do that.
>
Thanks for explanation, but I'm also know const pointer syntax.
> Incidentally, I agree with using a for-loop to copy the initial argument
> array instead of a memcpy; it makes the intent of the code clearer.
>
> OTOH, I really do not like code changes that cater to broken compilers.
>
I agree that making code worse just to make broken compilers happy is
not good thing, but I think new code is better and easier to read.
I was planning to fix compiler warning then I realized that code is
actually correct but could be rewritten clearer and fix this
false negative. That what I did in r1626247. But I forget to
update log message that I already wrote (it was part of batch fixes).
---
Ivan Zhakov
Re: svn commit: r1626247 - /subversion/trunk/subversion/libsvn_ra_svn/client.c
Posted by Branko Čibej <br...@wandisco.com>.
On 19.09.2014 17:21, ivan@apache.org wrote:
> Author: ivan
> Date: Fri Sep 19 15:21:15 2014
> New Revision: 1626247
>
> URL: http://svn.apache.org/r1626247
> Log:
> Resolve another compiler warning.
>
> * subversion/libsvn_ra_svn/client.c
> (find_tunnel_agent): Add non-const local variable ARGV to fix compiler
> warning.
Which compiler is that, and what warning does it emit? There is nothing
wrong with the code, and I've not seen any warnings from that code in
ages, with either gcc or clang. Specifically:
> -/* (Note: *ARGV is an output parameter.) */
> +/* (Note: *ARGV_P is an output parameter.) */
> static svn_error_t *find_tunnel_agent(const char *tunnel,
> const char *hostinfo,
> - const char ***argv,
> + const char ***argv_p,
> apr_hash_t *config, apr_pool_t *pool)
The original 'argv' parameter is *not* constant. It is a non-const
pointer to a non-const array of (const char*). There should be no
warnings when assigning to the pointer, and no warnings when modifying
the array. The only thing you can't do is modify the data the array
elements are pointing to, but the code did not do that.
Incidentally, I agree with using a for-loop to copy the initial argument
array instead of a memcpy; it makes the intent of the code clearer.
OTOH, I really do not like code changes that cater to broken compilers.
-- Brane