On Fri, Feb 28, 2020 at 03:13:35PM +0100, Tim Duesterhus wrote:
> `ist_NULL()` returns an `struct ist` with `.ptr = NULL` and `.len = 0`.
> ---
> include/common/ist.h | 8 +++++++-
> src/hpack-dec.c | 4 ++--
> src/http.c | 4 ++--
> src/http_htx.c | 2 +-
> 4 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/include/common/ist.h b/include/common/ist.h
> index 0d8b45721..4d75d8607 100644
> --- a/include/common/ist.h
> +++ b/include/common/ist.h
> @@ -183,6 +183,12 @@ static inline struct ist ist2(const void *ptr, size_t
> len)
> return (struct ist){ .ptr = (char *)ptr, .len = len };
> }
>
> +/* returns an `ist` with `.ptr = NULL` and `.len = 0` */
> +static inline struct ist ist_NULL()
> +{
> + return ist2(NULL, 0);
> +}
> +
> /* This function MODIFIES the string to add a zero AFTER the end, and returns
> * the start pointer. The purpose is to use it on strings extracted by
> parsers
> * from larger strings cut with delimiters that are not important and can be
> @@ -705,7 +711,7 @@ static inline struct ist istist(const struct ist ist,
> const struct ist pat)
> }
> return ist2(ret.ptr - 1, ret.len + 1);
> }
> - return ist2(NULL, 0);
> + return ist_NULL();
> }
I don't like much having to write something looking like a function call
to actually assign a constant, especially when building with -O0 where
this will really emit a function call. And yes I know that it's what we
currently have, except that currently it's a special case of a generic
function. If you're going to define something to assign a null, better
use a macro for this (and please drop the mixed case, that's painful to
type and even to spell loud). You can have:
#define IST_NULL ((const struct ist){ .ptr = 0, .len = 0 })
Then above you'd just do:
- return ist2(NULL, 0);
+ return IST_NULL;
I'd suggest also having a test for an ist's validity, because at many
places we explicitly test ist.ptr while actually thanks to the macros
and functions there are very few cases where this is still required.
We can for example have:
int isttest(const struct ist a)
{
return a.ptr != NULL;
}
I'd say that as a general rule, when you find it convenient to be able
to easily assign a special value, it's very likely that you'll want to
be able to easily test for this special value as well and it makes sense
to add the code for testing what you can assign.
Otherwise I'm all for doing it, the few changes in your code already
show that it's more readable.
Willy
