Re: [PATCH 3/3] wrapper: use a loop instead of repetitive statements
On 9/26/2019 10:45 PM, Jeff King wrote:
Show 33 quoted lines
> On Tue, Sep 24, 2019 at 08:01:58PM -0600, Alex Henrie wrote:
>
>> diff --git a/wrapper.c b/wrapper.c
>> index c55d7722d7..c23ac6adcd 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>> filename_template = &pattern[len - 6 - suffix_len];
>> for (count = 0; count < TMP_MAX; ++count) {
>> uint64_t v = value;
>> + int i;
>> /* Fill in the random bits. */
>> - filename_template[0] = letters[v % num_letters]; v /= num_letters;
>> - filename_template[1] = letters[v % num_letters]; v /= num_letters;
>> - filename_template[2] = letters[v % num_letters]; v /= num_letters;
>> - filename_template[3] = letters[v % num_letters]; v /= num_letters;
>> - filename_template[4] = letters[v % num_letters]; v /= num_letters;
>> - filename_template[5] = letters[v % num_letters]; v /= num_letters;
>> + for (i = 0; i < 6; i++) {
>> + filename_template[i] = letters[v % num_letters];
>> + v /= num_letters;
>> + }
>
> I'm not sure the readability is changed much either way. But it does
> enable this additional cleanup on top:
>
> -- >8 --
> Subject: git_mkstemps_mode(): replace magic numbers with computed value
>
> The magic number "6" appears several times in the function, and is
> related to the size of the "XXXXXX" string we expect to find in the
> template. Let's pull that "XXXXXX" into a constant array, whose size we
> can get at compile time with ARRAY_SIZE().Removing magic numbers is always a good change. Thanks!
Show 58 quoted lines
> Note that we probably can't just change this value, since callers will
> be feeding us a certain number of X's, but it hopefully makes the
> function itself easier to follow.
>
> While we're here, let's do the same with the "letters" array (which we
> _could_ modify if we wanted to include more characters).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> wrapper.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/wrapper.c b/wrapper.c
> index c23ac6adcd..e1eaef2e16 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -441,7 +441,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
> "abcdefghijklmnopqrstuvwxyz"
> "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> "0123456789";
> - static const int num_letters = 62;
> + static const int num_letters = ARRAY_SIZE(letters) - 1;
> + static const char x_pattern[] = "XXXXXX";
> + static const int num_x = ARRAY_SIZE(x_pattern) - 1;
> uint64_t value;
> struct timeval tv;
> char *filename_template;
> @@ -450,12 +452,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>
> len = strlen(pattern);
>
> - if (len < 6 + suffix_len) {
> + if (len < num_x + suffix_len) {
> errno = EINVAL;
> return -1;
> }
>
> - if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
> + if (strncmp(&pattern[len - num_x - suffix_len], x_pattern, num_x)) {
> errno = EINVAL;
> return -1;
> }
> @@ -466,12 +468,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
> */
> gettimeofday(&tv, NULL);
> value = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();
> - filename_template = &pattern[len - 6 - suffix_len];
> + filename_template = &pattern[len - num_x - suffix_len];
> for (count = 0; count < TMP_MAX; ++count) {
> uint64_t v = value;
> int i;
> /* Fill in the random bits. */
> - for (i = 0; i < 6; i++) {
> + for (i = 0; i < num_x; i++) {
> filename_template[i] = letters[v % num_letters];
> v /= num_letters;
> }
> Derrick Stolee <stolee@gmail.com>