Re: [PATCH v3 05/16] reftable: utility functions

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Nov 27, 2020, 10:18 UTC
Message-ID: <CAMP44s1EEwmJtQiPx=NkZdJ-7LOm+S55Hdukf+Oz1aBV1MREEg@mail.gmail.com>
In-Reply-To: <987d1d186fd68d604268f9a66118b7f276094daf.1606419752.git.gitgitgadget@gmail.com>

On Fri, Nov 27, 2020 at 2:42 AM Han-Wen Nienhuys via GitGitGadget <gitgitgadget@gmail.com> wrote:

> +int binsearch(size_t sz, int (*f)(size_t k, void *args), void *args)
> +{
> +       size_t lo = 0;
> +       size_t hi = sz;

Is there anything wrong with:

  size_t lo = 0, hi = sz;

> +       /* invariant: (hi == sz) || f(hi) == true
> +          (lo == 0 && f(0) == true) || fi(lo) == false
> +        */

This doesn't follow the multi-line format. And perhaps English would be better.

Show 9 quoted lines

> +       while (hi - lo > 1) {
> +               size_t mid = lo + (hi - lo) / 2;
> +
> +               int val = f(mid, args);
> +               if (val) {
> +                       hi = mid;
> +               } else {
> +                       lo = mid;
> +               }

Unnecessary braces are frowned upon. Also, why store the value if you are not going to use it later?

if (f(mid, args))
  hi = mid;
else
  lo = mid;

> +       }
> +
> +       if (lo == 0) {

if (!lo)

Show 5 quoted lines

> +               if (f(0, args)) {
> +                       return 0;
> +               } else {
> +                       return 1;
> +               }

return f(0, args) ? 0 : 1;

Or just:

return !f(0, args);

> +       }
> +
> +       return hi;

It would be much clearer to just do:

if (lo)
  return hi;
else
  return !f(0, args);

Show 8 quoted lines

> +}
> +
> +void free_names(char **a)
> +{
> +       char **p = a;
> +       if (p == NULL) {
> +               return;
> +       }

Once again:

  if (!p)
    return;

> +       while (*p) {
> +               reftable_free(*p);
> +               p++;
> +       }

This is the same thing as:

  if (!a)
    return;

  for (p = a; *p; p++)
    reftable_free(*p);

Show 10 quoted lines

> +int names_length(char **names)
> +{
> +       int len = 0;
> +       char **p = names;
> +       while (*p) {
> +               p++;
> +               len++;
> +       }
> +       return len;
> +}

Is there any need to keep track of len? It's always p - names.

This should do it:

  char **p;
  for (p = names; *p; p++);
  return p - names;

Show 9 quoted lines

> +void parse_names(char *buf, int size, char ***namesp)
> +{
> +       char **names = NULL;
> +       size_t names_cap = 0;
> +       size_t names_len = 0;
> +
> +       char *p = buf;
> +       char *end = buf + size;
> +       while (p < end) {

Yet another while that can be a for:

  for (p = buf; p < end; p = next + 1)

Show 6 quoted lines

> +               char *next = strchr(p, '\n');
> +               if (next != NULL) {
> +                       *next = 0;
> +               } else {
> +                       next = end;
> +               }

if (next)
  *next = 0;
else
  next = end;

> +               if (p < next) {

Or just:

  if (p >= next)
    continue;

> +                       if (names_len == names_cap) {
> +                               names_cap = 2 * names_cap + 1;
> +                               names = reftable_realloc(
> +                                       names, names_cap * sizeof(char *));

sizeof(*names);

Show 10 quoted lines

> +                       }
> +                       names[names_len++] = xstrdup(p);
> +               }
> +               p = next + 1;
> +       }
> +
> +       if (names_len == names_cap) {
> +               names_cap = 2 * names_cap + 1;
> +               names = reftable_realloc(names, names_cap * sizeof(char *));
> +       }

This is repeated, maybe a macro like ALLOC_GROW(), or literally ALLOC_GROW().

Show 14 quoted lines

> +       names[names_len] = NULL;
> +       *namesp = names;
> +}
> +
> +int names_equal(char **a, char **b)
> +{
> +       while (*a && *b) {
> +               if (strcmp(*a, *b)) {
> +                       return 0;
> +               }
> +
> +               a++;
> +               b++;
> +       }

Again:

  for (; *a && *b, a++, b++) {
    if (strcmp(*a, *b)
      return 0;
  }

But instead of moving multiple pointers:

  for (i = 0; a[i] && b[i], i++) {
    if (strcmp(a[i], b[i])
      return 0;
  }

> +int common_prefix_size(struct strbuf *a, struct strbuf *b)
> +{
> +       int p = 0;
> +       while (p < a->len && p < b->len) {

Once again, this can easily be a for.

Show 8 quoted lines

> +               if (a->buf[p] != b->buf[p]) {
> +                       break;
> +               }
> +               p++;
> +       }
> +
> +       return p;
> +}

Cheers.

-- 
Felipe Contreras