| Message ID | pull.1436.git.git.1673992498572.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git: replace strbuf_addstr with strbuf_addch for all strings of length 2 | expand |
"Rose via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Seija Kijin <doremylover123@gmail.com> > > This helps reduce overhead of calculating the length 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). > Subject: Re: [PATCH] git: replace strbuf_addstr with strbuf_addch for all strings of length 2 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. > diff --git a/builtin/am.c b/builtin/am.c > index 7e88d2426d7..c96886e0433 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -329,13 +329,11 @@ static void write_author_script(const struct am_state *state) > > strbuf_addstr(&sb, "GIT_AUTHOR_NAME="); > sq_quote_buf(&sb, state->author_name); > - strbuf_addch(&sb, '\n'); > > - strbuf_addstr(&sb, "GIT_AUTHOR_EMAIL="); > + strbuf_addstr(&sb, "\nGIT_AUTHOR_EMAIL="); > sq_quote_buf(&sb, state->author_email); > - strbuf_addch(&sb, '\n'); > > - strbuf_addstr(&sb, "GIT_AUTHOR_DATE="); > + strbuf_addstr(&sb, "\nGIT_AUTHOR_DATE="); > sq_quote_buf(&sb, state->author_date); > strbuf_addch(&sb, '\n'); 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. > diff --git a/builtin/blame.c b/builtin/blame.c > index 71f925e456c..3ab4cc0a56b 100644 > --- a/builtin/blame.c > +++ b/builtin/blame.c > @@ -143,11 +143,9 @@ static void get_ac_line(const char *inbuf, const char *what, > > if (split_ident_line(&ident, tmp, len)) { > error_out: > - /* Ugh */ > - tmp = "(unknown)"; > - strbuf_addstr(name, tmp); > - strbuf_addstr(mail, tmp); > - strbuf_addstr(tz, tmp); > + strbuf_addstr(name, "(unknown)"); > + strbuf_addstr(mail, "(unknown)"); > + strbuf_addstr(tz, "(unknown)"); 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. > if (opt->priv->call_depth) { > - strbuf_addchars(dest, ' ', 2); > - strbuf_addstr(dest, "From inner merge:"); > + strbuf_addstr(dest, " From inner merge:"); > strbuf_addchars(dest, ' ', opt->priv->call_depth * 2); Ditto, even though this is not as horrible as the change to builtin/am.c we saw earlier. I'll stop here.
On Wed, Jan 18, 2023 at 11:14 AM Junio C Hamano <gitster@pobox.com> wrote: > > From: Seija Kijin <doremylover123@gmail.com> > > This helps reduce overhead of calculating the length > > > diff --git a/builtin/am.c b/builtin/am.c > > strbuf_addstr(&sb, "GIT_AUTHOR_NAME="); > > sq_quote_buf(&sb, state->author_name); > > - strbuf_addch(&sb, '\n'); > > > > - strbuf_addstr(&sb, "GIT_AUTHOR_EMAIL="); > > + strbuf_addstr(&sb, "\nGIT_AUTHOR_EMAIL="); > > 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. Very much agree with this and all your other review comments. > > - strbuf_addchars(dest, ' ', 2); > > - strbuf_addstr(dest, "From inner merge:"); > > + strbuf_addstr(dest, " From inner merge:"); > > strbuf_addchars(dest, ' ', opt->priv->call_depth * 2); > > Ditto, even though this is not as horrible as the change to builtin/am.c > we saw earlier. Additionally, if this literal string ever gets wrapped in `_(...)`, then the above change is even more undesirable due to the extra burden it places on translators.
diff --git a/bisect.c b/bisect.c index ec7487e6836..ea534ad3777 100644 --- a/bisect.c +++ b/bisect.c @@ -443,7 +443,7 @@ static int register_ref(const char *refname, const struct object_id *oid, { struct strbuf good_prefix = STRBUF_INIT; strbuf_addstr(&good_prefix, term_good); - strbuf_addstr(&good_prefix, "-"); + strbuf_addch(&good_prefix, '-'); if (!strcmp(refname, term_bad)) { current_bad_oid = xmalloc(sizeof(*current_bad_oid)); diff --git a/builtin/am.c b/builtin/am.c index 7e88d2426d7..c96886e0433 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -329,13 +329,11 @@ static void write_author_script(const struct am_state *state) strbuf_addstr(&sb, "GIT_AUTHOR_NAME="); sq_quote_buf(&sb, state->author_name); - strbuf_addch(&sb, '\n'); - strbuf_addstr(&sb, "GIT_AUTHOR_EMAIL="); + strbuf_addstr(&sb, "\nGIT_AUTHOR_EMAIL="); sq_quote_buf(&sb, state->author_email); - strbuf_addch(&sb, '\n'); - strbuf_addstr(&sb, "GIT_AUTHOR_DATE="); + strbuf_addstr(&sb, "\nGIT_AUTHOR_DATE="); sq_quote_buf(&sb, state->author_date); strbuf_addch(&sb, '\n'); diff --git a/builtin/blame.c b/builtin/blame.c index 71f925e456c..3ab4cc0a56b 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -143,11 +143,9 @@ static void get_ac_line(const char *inbuf, const char *what, if (split_ident_line(&ident, tmp, len)) { error_out: - /* Ugh */ - tmp = "(unknown)"; - strbuf_addstr(name, tmp); - strbuf_addstr(mail, tmp); - strbuf_addstr(tz, tmp); + strbuf_addstr(name, "(unknown)"); + strbuf_addstr(mail, "(unknown)"); + strbuf_addstr(tz, "(unknown)"); *time = 0; return; } diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index c3ea09281af..73b755029ee 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -60,7 +60,7 @@ static void expand_objectsize(struct strbuf *line, const struct object_id *oid, } else if (padded) { strbuf_addf(line, "%7s", "-"); } else { - strbuf_addstr(line, "-"); + strbuf_addch(line, '-'); } } diff --git a/diff.c b/diff.c index 329eebf16a0..b379660c42b 100644 --- a/diff.c +++ b/diff.c @@ -1702,7 +1702,7 @@ static void add_line_count(struct strbuf *out, int count) strbuf_addstr(out, "0,0"); break; case 1: - strbuf_addstr(out, "1"); + strbuf_addch(out, '1'); break; default: strbuf_addf(out, "1,%d", count); diff --git a/log-tree.c b/log-tree.c index 1dd5fcbf7be..23f2a62c5ac 100644 --- a/log-tree.c +++ b/log-tree.c @@ -388,7 +388,7 @@ void fmt_output_subject(struct strbuf *filename, strbuf_addf(&temp, "v%s", info->reroll_count); format_sanitized_subject(filename, temp.buf, temp.len); - strbuf_addstr(filename, "-"); + strbuf_addch(filename, '-'); strbuf_release(&temp); } strbuf_addf(filename, "%04d-%s", nr, subject); diff --git a/merge-ort.c b/merge-ort.c index d1611ca400a..3132ac22aba 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -801,8 +801,7 @@ static void path_msg(struct merge_options *opt, va_start(ap, fmt); if (opt->priv->call_depth) { - strbuf_addchars(dest, ' ', 2); - strbuf_addstr(dest, "From inner merge:"); + strbuf_addstr(dest, " From inner merge:"); strbuf_addchars(dest, ' ', opt->priv->call_depth * 2); } strbuf_vaddf(dest, fmt, ap); diff --git a/path.c b/path.c index 492e17ad121..05d3b6d9059 100644 --- a/path.c +++ b/path.c @@ -1082,7 +1082,7 @@ const char *remove_leading_path(const char *in, const char *prefix) strbuf_reset(&buf); if (!in[j]) - strbuf_addstr(&buf, "."); + strbuf_addch(&buf, '.'); else strbuf_addstr(&buf, in + j); return buf.buf; diff --git a/protocol-caps.c b/protocol-caps.c index bbde91810ac..80ec75e1131 100644 --- a/protocol-caps.c +++ b/protocol-caps.c @@ -63,7 +63,7 @@ static void send_info(struct repository *r, struct packet_writer *writer, if (info->size) { if (oid_object_info(r, &oid, &object_size) < 0) { - strbuf_addstr(&send_buffer, " "); + strbuf_addch(&send_buffer, ' '); } else { strbuf_addf(&send_buffer, " %lu", object_size); } diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c index 469ab79a5ad..ed0f6058ba9 100644 --- a/reftable/readwrite_test.c +++ b/reftable/readwrite_test.c @@ -483,7 +483,7 @@ static void test_table_read_write_seek(int index, int hash_id) } strbuf_addstr(&pastLast, names[N - 1]); - strbuf_addstr(&pastLast, "/"); + strbuf_addch(&pastLast, '/'); err = reftable_reader_seek_ref(&rd, &it, pastLast.buf); if (err == 0) { diff --git a/reftable/refname.c b/reftable/refname.c index 95734969324..b6d5b76a8fe 100644 --- a/reftable/refname.c +++ b/reftable/refname.c @@ -179,7 +179,7 @@ int modification_validate(struct modification *mod) goto done; strbuf_reset(&slashed); strbuf_addstr(&slashed, mod->add[i]); - strbuf_addstr(&slashed, "/"); + strbuf_addch(&slashed, '/'); err = modification_has_ref_with_prefix(mod, slashed.buf); if (err == 0) { diff --git a/reftable/stack.c b/reftable/stack.c index ddbdf1b9c8b..479658e428d 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -35,7 +35,7 @@ static void stack_filename(struct strbuf *dest, struct reftable_stack *st, { strbuf_reset(dest); strbuf_addstr(dest, st->reftable_dir); - strbuf_addstr(dest, "/"); + strbuf_addch(dest, '/'); strbuf_addstr(dest, name); } @@ -547,11 +547,11 @@ int reftable_addition_commit(struct reftable_addition *add) for (i = 0; i < add->stack->merged->stack_len; i++) { strbuf_addstr(&table_list, add->stack->readers[i]->name); - strbuf_addstr(&table_list, "\n"); + strbuf_addch(&table_list, '\n'); } for (i = 0; i < add->new_tables_len; i++) { strbuf_addstr(&table_list, add->new_tables[i]); - strbuf_addstr(&table_list, "\n"); + strbuf_addch(&table_list, '\n'); } err = write(add->lock_file_fd, table_list.buf, table_list.len); @@ -1013,15 +1013,15 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, for (i = 0; i < first; i++) { strbuf_addstr(&ref_list_contents, st->readers[i]->name); - strbuf_addstr(&ref_list_contents, "\n"); + strbuf_addch(&ref_list_contents, '\n'); } if (!is_empty_table) { strbuf_addbuf(&ref_list_contents, &new_table_name); - strbuf_addstr(&ref_list_contents, "\n"); + strbuf_addch(&ref_list_contents, '\n'); } for (i = last + 1; i < st->merged->stack_len; i++) { strbuf_addstr(&ref_list_contents, st->readers[i]->name); - strbuf_addstr(&ref_list_contents, "\n"); + strbuf_addch(&ref_list_contents, '\n'); } err = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len); diff --git a/reftable/stack_test.c b/reftable/stack_test.c index d0b717510fa..4fcaefd3ddb 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -181,7 +181,7 @@ static void test_reftable_stack_add_one(void) strbuf_reset(&scratch); strbuf_addstr(&scratch, dir); - strbuf_addstr(&scratch, "/"); + strbuf_addch(&scratch, '/'); /* do not try at home; not an external API for reftable. */ strbuf_addstr(&scratch, st->readers[0]->name); err = stat(scratch.buf, &stat_result); diff --git a/reftable/writer.c b/reftable/writer.c index 2e322a5683d..61d6f3229f3 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -351,7 +351,7 @@ int reftable_writer_add_log(struct reftable_writer *w, err = REFTABLE_API_ERROR; goto done; } - strbuf_addstr(&cleaned_message, "\n"); + strbuf_addch(&cleaned_message, '\n'); log->value.update.message = cleaned_message.buf; } diff --git a/sequencer.c b/sequencer.c index bcb662e23be..27f41a027d2 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2233,7 +2233,7 @@ static int do_pick_commit(struct repository *r, } else { strbuf_addstr(&msgbuf, "Revert \""); strbuf_addstr(&msgbuf, msg.subject); - strbuf_addstr(&msgbuf, "\""); + strbuf_addch(&msgbuf, '\"'); } strbuf_addstr(&msgbuf, "\n\nThis reverts commit "); refer_to_commit(opts, &msgbuf, commit); diff --git a/setup.c b/setup.c index cefd5f63c46..865ed8fbe98 100644 --- a/setup.c +++ b/setup.c @@ -1349,7 +1349,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, return GIT_DIR_DISALLOWED_BARE; if (!ensure_valid_ownership(NULL, NULL, dir->buf, report)) return GIT_DIR_INVALID_OWNERSHIP; - strbuf_addstr(gitdir, "."); + strbuf_addch(gitdir, '.'); return GIT_DIR_BARE; } diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c index fbbef68dfc0..e99ee58ca38 100644 --- a/trace2/tr2_tgt_normal.c +++ b/trace2/tr2_tgt_normal.c @@ -224,7 +224,7 @@ static void fn_child_start_fl(const char *file, int line, if (cmd->dir) { strbuf_addstr(&buf_payload, " cd "); sq_quote_buf_pretty(&buf_payload, cmd->dir); - strbuf_addstr(&buf_payload, ";"); + strbuf_addch(&buf_payload, ';'); } /*