`git_config_string()` output parameter `dest` is declared as a const
which is unnecessary as the caller of the function is given a strduped
string which can be modified without causing any harm.
Thus, remove the const from the function signature.
Signed-off-by: Tanay Abhra <redacted>
---
cache.h | 2 +-
config.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 92fc9f1..93d357a 100644
--- a/cache.h
+++ b/cache.h
@@ -1303,7 +1303,7 @@ extern unsigned long git_config_ulong(const char *, const char *);
extern int git_config_bool_or_int(const char *, const char *, int *);
extern int git_config_bool(const char *, const char *);
extern int git_config_maybe_bool(const char *, const char *);
-extern int git_config_string(const char **, const char *, const char *);
+extern int git_config_string(char **, const char *, const char *);
extern int git_config_pathname(const char **, const char *, const char *);
extern int git_config_set_in_file(const char *, const char *, const char *);
extern int git_config_set(const char *, const char *);
diff --git a/config.c b/config.c
index ba882a1..25e28a7 100644
--- a/config.c
+++ b/config.c
@@ -633,7 +633,7 @@ int git_config_bool(const char *name, const char *value)
return !!git_config_bool_or_int(name, value, &discard);
}
-int git_config_string(const char **dest, const char *var, const char *value)
+int git_config_string(char **dest, const char *var, const char *value)
{
if (!value)
return config_error_nonbool(var);--
1.9.0.GIT
On Tue, Jul 22, 2014 at 03:49:56AM -0700, Tanay Abhra wrote:
`git_config_string()` output parameter `dest` is declared as a const
which is unnecessary as the caller of the function is given a strduped
string which can be modified without causing any harm.
Thus, remove the const from the function signature.
You are correct that it is unnecessary. However, this patch alone is not
sufficient because of the way const-ness in C works. If I have:
static const char *some_global;
then with your patch, calling:
git_config_string(&some_global, var, value);
will complain that we are passing a pointer to "const char *", not a
pointer to "char *". And indeed, compiling with your patch introduces a
ton of compiler warnings.
We would have to convert each of the variables we pass to it to:
static char *some_global;
That's not so bad, but:
static char *some_global = "some_default_value";
is wrong. Such a global sometimes points to const storage (i.e.,
initially), and sometimes to allocated storage (if it was loaded from
config). We simply keep the latter as a const pointer (since we would
not bother to free it at the end of the program anyway), and that
decision influences git_config_string, which is just a helper for
setting such variables anyway.
So I would not mind lifting this unnecessary restriction on
git_config_string, but I do not see a way to do it without making the
rest of the code much uglier (and I do not see a particular advantage in
modifying git_config_string here that would make it worth the trouble).
-Peff
On 7/22/2014 4:37 PM, Jeff King wrote:
On Tue, Jul 22, 2014 at 03:49:56AM -0700, Tanay Abhra wrote:
quoted
`git_config_string()` output parameter `dest` is declared as a const
which is unnecessary as the caller of the function is given a strduped
string which can be modified without causing any harm.
Thus, remove the const from the function signature.
You are correct that it is unnecessary. However, this patch alone is not
sufficient because of the way const-ness in C works. If I have:
static const char *some_global;
then with your patch, calling:
git_config_string(&some_global, var, value);
will complain that we are passing a pointer to "const char *", not a
pointer to "char *". And indeed, compiling with your patch introduces a
ton of compiler warnings.
I had also thought that the compiler would raise lot of warnings but it didn't
on the first compile.
Now I checked again and now it complains a lot, maybe it because I was tinkering
with my config.mak, dunno.
We would have to convert each of the variables we pass to it to:
static char *some_global;
That's not so bad, but:
static char *some_global = "some_default_value";
is wrong. Such a global sometimes points to const storage (i.e.,
initially), and sometimes to allocated storage (if it was loaded from
config). We simply keep the latter as a const pointer (since we would
not bother to free it at the end of the program anyway), and that
decision influences git_config_string, which is just a helper for
setting such variables anyway.
So I would not mind lifting this unnecessary restriction on
git_config_string, but I do not see a way to do it without making the
rest of the code much uglier (and I do not see a particular advantage in
modifying git_config_string here that would make it worth the trouble).
Yes, you are right. This patch is the conclusion of discussion in [1].
I used the same function signature as git_config_string for
git_config_get_string() which lead to some ugly casts like
+git_config_get_string("imap.folder", (const char**)&imap_folder);
in imap-send.c patch and others. What should we do about such cases, I used
either an intermediate variable or casts but Junio commented that it would be better
if the dest parameter was a non-const and that it was a weakness of the config-set
API that demanded the dest to be a const pointer.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/253948/