Re: [PATCH 3/4] strbuf: introduce `strbuf_attachstr()`

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Martin Ågren <martin.agren@xxxxxxxxx> writes:

> +/**
> + * Attach a string to a buffer similar to `strbuf_attachstr_len()`.
> + * Useful if you do not know the length of the string.
> + */
> +static inline void strbuf_attachstr(struct strbuf *sb, char *str)
> +{
> +	size_t len = strlen(str);
> +
> +	strbuf_attach(sb, str, len, len + 1);
> +}

This is somewhat worrysome in that the interface is _so_ simple that
people may fail to see that str must be allocated piece of memory,
and it is preferrable if string fully fills the allocation.

We should repeat that (instead of just trusting "similar to ..."
would tell them enough) in the doc, perhaps?

> diff --git a/trailer.c b/trailer.c
> index 0c414f2fed..56c4027943 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1095,7 +1095,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
>  	for (ptr = trailer_lines; *ptr; ptr++) {
>  		if (last && isspace((*ptr)->buf[0])) {
>  			struct strbuf sb = STRBUF_INIT;
> -			strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
> +			strbuf_attachstr(&sb, *last);
>  			strbuf_addbuf(&sb, *ptr);
>  			*last = strbuf_detach(&sb, NULL);
>  			continue;

This is not wrong per-se, but it is unclear if use of strbuf_attach*
family to avoid an explicit malloc/copy/free is buying much at this
callsite.  Simplifying the code here of course is not within the
scope of this series.





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux