Re: [PATCH v5 1/6] strbuf: add shortcut to work with error messages
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 21, 2018, 20:20 UTC
- Message-ID
- <xmqqlgelnpsx.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000000@eu-west-1.amazonses.com>
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
Show 23 quoted lines
> Add function strbuf_error() that helps to save few lines of code.
> Function expands fmt with placeholders, append resulting error message
> to strbuf *err, and return error code ret.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
> strbuf.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/strbuf.h b/strbuf.h
> index e6cae5f4398c8..fa66d4835f1a7 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -620,4 +620,17 @@ char *xstrvfmt(const char *fmt, va_list ap);
> __attribute__((format (printf, 1, 2)))
> char *xstrfmt(const char *fmt, ...);
>
> +/*
> + * Expand error message, append it to strbuf *err, then return error code ret.
> + * Allow to save few lines of code.
> + */
> +static inline int strbuf_error(struct strbuf *err, int ret, const char *fmt, ...)
> +{With this function, err does not have to be an error message, and ret does not have to be negative. Hence strbuf_error() is a wrong name for the wrapper.
It somewhat is bothersome to see that this is inlined; if it is meant for error codepath, it probably shouldn't have to be.
Show 8 quoted lines
> + va_list ap; > + va_start(ap, fmt); > + strbuf_vaddf(err, fmt, ap); > + va_end(ap); > + return ret; > +} > + > #endif /* STRBUF_H */
Quite honestly, I am not sure if it is worth to be in strbuf.h; it feels a bit too specific to the immediate need for these five patches and nowhere else. Are there many existing calls to strbuf_addf() immediately followed by an "return" of an integer, that can be simplified by using this helper? I see quite a many instances of addf() soon followed by "return -1" in refs/files-backend.c, but they are not immediately adjacent to each other, and won't be helped.