| Message ID | 283f502acb68910cb43d6077eef99d6345aaea4b.1698791220.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid passing global comment_line_char repeatedly | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > -static void add_lines(struct strbuf *out, > - const char *prefix1, > - const char *prefix2, > - const char *buf, size_t size) > +void strbuf_add_lines_varied_prefix(struct strbuf *sb, > + const char *default_prefix, > + const char *tab_nl_prefix, > + const char *buf, size_t size) > { > while (size) { > const char *prefix; > const char *next = memchr(buf, '\n', size); > next = next ? (next + 1) : (buf + size); > > - prefix = ((prefix2 && (buf[0] == '\n' || buf[0] == '\t')) > - ? prefix2 : prefix1); > - strbuf_addstr(out, prefix); > - strbuf_add(out, buf, next - buf); > + prefix = (buf[0] == '\n' || buf[0] == '\t') > + ? tab_nl_prefix : default_prefix; > + strbuf_addstr(sb, prefix); > + strbuf_add(sb, buf, next - buf); The original allowed callers to pass NULL for the second prefix when they want to use the same prefix, even for commenting out an empty line or a line that begins with a tab. The new one does not allow the callers to do so. As long as updating the existing callers are done carefully, the difference would not matter, but would it help new callers in the future to rid the usability feature like this patch does while performing a refactoring? The loss of feature is not even documented, by the way. While "tab_nl" sound a bit more specific than "2", I am not sure if we made it better. It does not make it clear why it makes sense to (and it is necessary to) special case HT and LF. A developer who is writing a new caller would not know why there are two prefixes supported, or why the function is named "varied prefix", with these names. Giving a name that explains the reason might help the readability. I've been thinking what the best name for this function would be but not successfully. It may be that we shouldn't take two prefixes in the first place. The ONLY case callers want to pass prefix2 that is different from prefix1 is when prefix1 ends with a space, and prefix2 is identical to prefix1 without the trailing space. The reason they use such a pair of prefixes is to avoid leaving a trailing whitespace (when buf[0] == '\n') or having a space before tab (when buf[0] == '\t') on the generated lines. So eventually we may want to have something like this as the final interface given to the public callers, simply because ... strbuf_add_lines_as_comments(struct strbuf *sb, const char *comment_prefix, const char *buf, size_t size) { while (size) { const char *next = memchr(buf, '\n', size); next = next ? (next + 1) : (buf + size); strbuf_addstr(sb, comment_prefix); /* avoid trailing-whitespace and space-before-tab */ if (buf[0] != '\n' && buf[0] != '\t') strbuf_addch(sb, ' '); strbuf_add(sb, buf, next - buf); ... loop control ... } ... strbuf completion ... } ... there is no need for totally unrelated two prefix variants. And both the function name and the parameter name would be a bit easier to understand than your version (and far easier than the original). The function is about commenting out all the lines in buf with the comment prefix, and most of the time we add a space between the comment character and the commented out text, but in some cases we do not want to add the space. But as I said already, I'd prefer to see a patch that claims to be a refactoring to do as little as necessary. Giving it a name better than add_lines() is inevitable, because you are making it extern. But I'd prefer to see the parameter naems and the function body left untouched and kept the same as the original. It should be left to a separate step to improve the interface and the implementation. Thanks.
diff --git a/strbuf.c b/strbuf.c index 2088f7800a..d5ee8874f8 100644 --- a/strbuf.c +++ b/strbuf.c @@ -340,24 +340,24 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...) va_end(ap); } -static void add_lines(struct strbuf *out, - const char *prefix1, - const char *prefix2, - const char *buf, size_t size) +void strbuf_add_lines_varied_prefix(struct strbuf *sb, + const char *default_prefix, + const char *tab_nl_prefix, + const char *buf, size_t size) { while (size) { const char *prefix; const char *next = memchr(buf, '\n', size); next = next ? (next + 1) : (buf + size); - prefix = ((prefix2 && (buf[0] == '\n' || buf[0] == '\t')) - ? prefix2 : prefix1); - strbuf_addstr(out, prefix); - strbuf_add(out, buf, next - buf); + prefix = (buf[0] == '\n' || buf[0] == '\t') + ? tab_nl_prefix : default_prefix; + strbuf_addstr(sb, prefix); + strbuf_add(sb, buf, next - buf); size -= next - buf; buf = next; } - strbuf_complete_line(out); + strbuf_complete_line(sb); } void strbuf_add_commented_lines(struct strbuf *out, @@ -370,7 +370,7 @@ void strbuf_add_commented_lines(struct strbuf *out, xsnprintf(prefix1, sizeof(prefix1), "%c ", comment_line_char); xsnprintf(prefix2, sizeof(prefix2), "%c", comment_line_char); } - add_lines(out, prefix1, prefix2, buf, size); + strbuf_add_lines_varied_prefix(out, prefix1, prefix2, buf, size); } void strbuf_commented_addf(struct strbuf *sb, @@ -751,7 +751,7 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint) void strbuf_add_lines(struct strbuf *out, const char *prefix, const char *buf, size_t size) { - add_lines(out, prefix, NULL, buf, size); + strbuf_add_lines_varied_prefix(out, prefix, prefix, buf, size); } void strbuf_addstr_xml_quoted(struct strbuf *buf, const char *s) diff --git a/strbuf.h b/strbuf.h index 4547efa62e..a9333ac1ad 100644 --- a/strbuf.h +++ b/strbuf.h @@ -601,6 +601,10 @@ void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size); +void strbuf_add_lines_varied_prefix(struct strbuf *sb, + const char *default_prefix, + const char *tab_nl_prefix, + const char *buf, size_t size); /** * Append s to sb, with the characters '<', '>', '&' and '"' converted
A subsequent patch will require the ability to add different prefixes to different lines (depending on their contents), so make this functionality available from outside strbuf.c. The function name is chosen to avoid a conflict with the existing function named strbuf_add_lines(). Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- strbuf.c | 22 +++++++++++----------- strbuf.h | 4 ++++ 2 files changed, 15 insertions(+), 11 deletions(-)