Re: [PATCH v5 1/6] strbuf: add shortcut to work with error messages

[Оля Тележная]

2018-03-21 23:20 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> 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 
>> ---
>>  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.
>
>> + 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.

Summarizing all that we discussed: I have 2 options how to continue
this patch. I can revert to v4, or I can replace new function in
strbuf.h with similar macro in ref-filter.c (I mean, there would be no
changes in strbuf.h and one new macro in ref-filter.c). What do you
like more?

Thanks!
Olga

Re: [PATCH v5 1/6] strbuf: add shortcut to work with error messages

[Junio C Hamano]

Olga Telezhnaya  writes:

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

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

[PATCH v5 1/6] strbuf: add shortcut to work with error messages

[Olga Telezhnaya]

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 
---
 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, 
...)
+{
+   va_list ap;
+   va_start(ap, fmt);
+   strbuf_vaddf(err, fmt, ap);
+   va_end(ap);
+   return ret;
+}
+
 #endif /* STRBUF_H */

--
https://github.com/git/git/pull/466