Re: [PATCH] git: replace strbuf_addstr with strbuf_addch for all strings of length 2

"Rose via GitGitGadget" <gitgitgadget@gmail.com> writes:

Have you measured how much overhead this change is saving, or is this a 300 line e-mail message that churns code without giving us any measurable improvement?

There also seem to be some totally unrelated changes of dubious merit hidden in these patches (see below).

I think you are touching strings of length 1, not 2. Running strlen() on such a string returns will return 1 without counting the terminating NUL.

This may reduce the number of lines, but markedly worsens the readability of the resulting code. Each of the three-line blocks in the original used to be logically complete and independent unit, but now each of them depend on what the last block wants.

In any case, this has nothing to do with "addstr() of constant string can be replaced with add() with a constant length or addch() of a sing character to make it unnecessary to compute the length".

By the way, the current implementation of strbuf_addstr() looks like this:

        static inline void strbuf_addstr(struct strbuf *sb, const char *s)
        {
                strbuf_add(sb, s, strlen(s));
        }

Decent optimizing compilers should be able to see through the code like this you write:

	strbuf_addstr(&sb, "constant");

which becomes

	strbuf_add(&sb, "constant", strlen("constant"))

when inlined, and realize strlen("constant") can be computed at compile time, turning it into

	strbuf_add(&sb, "constant", 8);

That way, people can make their strbuf_addstr() calls in the way they find the most natural, i.e. without having to choose between _addstr() and _add(). If the compilers can do their unnecessary thinking for us humans, we should make them do so to help us.

If somebody can prove that between these two

	strbuf_add(&sb, "c", 1);
	strbuf_addch(&sb, 'c');

there is a meaningful difference in overhead to encourage us to rewrite the former to the latter, perhaps a similar trick can be employed in the implementation of strbuf_add(), perhaps like:

        static inline void strbuf_add(struct strbuf *sb, const void *data, size_t len)
        {
                if (len == 1) {
                        strbuf_addch(sb, *((char *)sb));
                } else {
                        strbuf_grow(sb, len);
                        memcpy(sb->buf + sb->len, data, len);
                        strbuf_setlen(sb, sb->len + len);
                }
        }

That way, people can make their strbuf_add() and strbuf_addstr() calls in the way they find the most natural, i.e. without having to choose between _add() and _addch() depending on the length of the string. This makes the code easier to maintain, as we do not have to change the code all that much when the length of the string we need to append to a strbuf changes from 1 to more (or the other way around).

But I somehow doubt it is worth it.

This is another unrelated change that has not much to do with the theme of the change, to use addch() when the string is of length 1.

Ditto, even though this is not as horrible as the change to builtin/am.c we saw earlier.

I'll stop here.