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