Re: [PATCH] config.c: change the function signature of `git_config_string()`

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]