[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 2/8] gzip: refactor and expand macros
On 24.04.2024 18:34, Daniel P. Smith wrote: > This commit refactors macros into proper static functions. It in-place expands > the `flush_output` macro, allowing the clear removal of the dead code > underneath the `underrun` label. But it's NEXTBYTE() which uses the label, not flush_output(). I'm actually unconvinced of its expanding / removal. > --- a/xen/common/gzip/gunzip.c > +++ b/xen/common/gzip/gunzip.c > @@ -25,8 +25,6 @@ typedef unsigned char uch; > typedef unsigned short ush; > typedef unsigned long ulg; > > -#define get_byte() (inptr < insize ? inbuf[inptr++] : fill_inbuf()) > - > /* Diagnostic functions */ > #ifdef DEBUG > # define Assert(cond, msg) do { if (!(cond)) error(msg); } while (0) > @@ -52,10 +50,14 @@ static __init void error(const char *x) > panic("%s\n", x); > } > > -static __init int fill_inbuf(void) > -{ > - error("ran out of input data"); > - return 0; I'm not convinced of the removal of this as a separate function. It only calling error() right now could change going forward, so I'd at least expect a little bit of a justification. > +static __init uch get_byte(void) { Nit: Brace goes on its own line. Also for (effectively) new code it would be nice if __init (and alike) would be placed "canonically", i.e. between type and identifier. > --- a/xen/common/gzip/inflate.c > +++ b/xen/common/gzip/inflate.c > @@ -119,6 +119,18 @@ static char rcsid[] = "#Id: inflate.c,v 0.14 1993/06/10 > 13:27:04 jloup Exp #"; > > #endif /* !__XEN__ */ > > +/* > + * The inflate algorithm uses a sliding 32 K byte window on the uncompressed > + * stream to find repeated byte strings. This is implemented here as a > + * circular buffer. The index is updated simply by incrementing and then > + * ANDing with 0x7fff (32K-1). > + * > + * It is left to other modules to supply the 32 K area. It is assumed > + * to be usable as if it were declared "uch slide[32768];" or as just > + * "uch *slide;" and then malloc'ed in the latter case. The definition > + * must be in unzip.h, included above. Nit: s/definition/declaration/ ? > + */ > +#define wp outcnt > #define slide window > > /* > @@ -150,21 +162,6 @@ static int inflate_dynamic(void); > static int inflate_block(int *); > static int inflate(void); > > -/* > - * The inflate algorithm uses a sliding 32 K byte window on the uncompressed > - * stream to find repeated byte strings. This is implemented here as a > - * circular buffer. The index is updated simply by incrementing and then > - * ANDing with 0x7fff (32K-1). > - * > - * It is left to other modules to supply the 32 K area. It is assumed > - * to be usable as if it were declared "uch slide[32768];" or as just > - * "uch *slide;" and then malloc'ed in the latter case. The definition > - * must be in unzip.h, included above. Oh, an earlier comment just moves up. Is there really a need for this extra churn? > @@ -224,7 +221,7 @@ static const ush mask_bits[] = { > 0x01ff, 0x03ff, 0x07ff, 0x0fff, 0x1fff, 0x3fff, 0x7fff, 0xffff > }; > > -#define NEXTBYTE() ({ int v = get_byte(); if (v < 0) goto underrun; (uch)v; > }) > +#define NEXTBYTE() (get_byte()) /* get_byte will panic on failure */ Nit: No need for the outer parentheses. > @@ -1148,8 +1135,8 @@ static int __init gunzip(void) > NEXTBYTE(); > NEXTBYTE(); > > - (void)NEXTBYTE(); /* Ignore extra flags for the moment */ > - (void)NEXTBYTE(); /* Ignore OS type for the moment */ > + NEXTBYTE(); /* Ignore extra flags for the moment */ > + NEXTBYTE(); /* Ignore OS type for the moment */ In Misra discussions there were indications that such casts may need (re-) introducing. Perhaps better leave this alone, the more when it's not really fitting the patch's purpose? Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |