On Fri, Nov 27, 2020 at 2:42 AM Han-Wen Nienhuys via GitGitGadget
<gitgitgadget@xxxxxxxxx> 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.
> + 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)
> + 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);
> +}
> +
> +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);
> +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;
> +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)
> + 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);
> + }
> + 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().
> + 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.
> + if (a->buf[p] != b->buf[p]) {
> + break;
> + }
> + p++;
> + }
> +
> + return p;
> +}
Cheers.
--
Felipe Contreras
