Re: [PATCH v5 7/7] strbuf: remove global variable

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 11, 2023, 20:24 UTC
Message-ID: <CAPig+cR-m45q0Y9YUnRPRzny0Q=sa4dPWCjxwHBN84VMtEsxzA@mail.gmail.com>
In-Reply-To: <20230511194822.1493798-7-calvinwan@google.com>

On Thu, May 11, 2023 at 3:48 PM Calvin Wan <calvinwan@google.com> wrote:

Show 16 quoted lines

> 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@google.com>
> ---
> 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.

Show 12 quoted lines

> 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.