On Thu, May 11, 2023 at 3:48 PM Calvin Wan <calvinwan@xxxxxxxxxx> wrote:
> As a library that only interacts with other primitives, strbuf should
> not utilize the comment_line_char global variable within its
> functions. Therefore, add an additional parameter for functions that use
> comment_line_char and refactor callers to pass it in instead.
> strbuf_stripspace() removes the skip_comments boolean and checks if
> comment_line_char is a non-NULL character to determine whether to skip
> comments or not.
>
> Signed-off-by: Calvin Wan <calvinwan@xxxxxxxxxx>
> ---
> diff --git a/strbuf.c b/strbuf.c
> @@ -1051,10 +1052,10 @@ static size_t cleanup(char *line, size_t len)
> - * Enable skip_comments to skip every line starting with comment
> - * character.
> + * Pass a non-NULL comment_line_char to skip every line starting
> + * with it
When speaking of a character, we normally spell this NUL rather than
NULL (which is how we spell a null pointer).
The end-of-sentence period got lost in the rewrite.
Probably not worth a reroll on its own.
> @@ -1067,7 +1068,8 @@ void strbuf_stripspace(struct strbuf *sb, int skip_comments)
> - if (skip_comments && len && sb->buf[i] == comment_line_char) {
> + if (comment_line_char != '\0' && len &&
> + sb->buf[i] == comment_line_char) {
In this code-base, it is more common to say:
if (comment_line_char && len && ...
rather than:
if (comment_line_char != '\0' && len && ...
Probably not worth a reroll.
> diff --git a/strbuf.h b/strbuf.h
> @@ -544,10 +545,12 @@ int strbuf_getcwd(struct strbuf *sb);
> /**
> - * Strip whitespace from a buffer. The second parameter controls if
> - * comments are considered contents to be removed or not.
> + * Strip whitespace from a buffer. The second parameter is the comment
> + * character that determines whether a line is a comment or not. If the
> + * second parameter is a non-NULL character, comments are considered
> + * contents to be removed.
> */
> -void strbuf_stripspace(struct strbuf *buf, int skip_comments);
> +void strbuf_stripspace(struct strbuf *buf, char comment_line_char);
Ditto regarding NUL vs. NULL.
Rather than saying "the second parameter", readers would have an
easier time if you just spelled it out directly (i.e.
"comment_line_char").
The phrase "comments are considered contents to be removed" made sense
before the rewrite, but sounds a bit awkward after the rewrite.
Perhaps rewrite it something like this:
Strip whitespace from a buffer. If comment_line_char is non-NUL,
then lines beginning with that character are considered comments,
thus removed.
May not be worth a reroll.
