Re: [PATCH 2/5] gzip: clean up comments and fix code alignment

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 4/11/24 15:11, Andrew Cooper wrote:
> On 11/04/2024 4:25 pm, Daniel P. Smith wrote:
> > This commit cleans up the comments and fixes the code alignment using Xen
> > coding style. This is done to make the code more legible before refactoring.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> I've found two more minor adjustments:
>
> diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
> index feb6d51008aa..9205189d4618 100644
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -375,7 +375,7 @@ static int __init huft_build(
>       memzero(stk->c, sizeof(stk->c));
>       p = b;  i = n;
>       do {
> -        Tracecv(*p, (stderr, (n-i >= ' ' && n-i <= '~' ? "%c %d\n" :
> "0x%x %d\n"),
> +        Tracecv(*p, (stderr, (n-i >= ' ' && n-i <= '~' ? "%c %d\n" :
> "0x%x %d\n"),
>                        n-i, *p));
>           c[*p]++;                    /* assume all entries <= BMAX */
>           p++;                      /* Can't combine with above line
> (Solaris bug) */
> @@ -563,7 +563,8 @@ static int __init huft_build(
>       return ret;
>   }
>   
> -/* Free the malloc'ed tables built by huft_build(), which makes a linked
> +/*
> + * Free the malloc'ed tables built by huft_build(), which makes a linked
>    * list of the tables it made, with the links in a dummy first entry of
>    * each table.
>    *
>
>
> I can fold on commit.  (First hunk is trailing whitespace, which doesn't
> show up so well on email).

Hmm, I tried to catch all the trailing space. Since I already have to 
respin for the MISRA exclude list, I can double-check if there are any 
other trailing whitespaces I missed.
> However, there are some more major adjustments wanted too.
That's fine, I attempted at tugging on some of the ugliness and kept 
finding myself in a mess. If there are subtle improvements that can be 
made without doing the complete rewrite I think this really deserves, 
would be glad to incorporate them.
> The reason why the code indention is so messed up is because it has been
> auto-formatted, but with some NEXTBYTE()/NEEDBITS()/DUMPBITS() missing
> semi-colons.  This throws off subsequent formatting, including some of
> the indentation changes you've made.
>
> Fixing the semicolons is a far more messy diff, but a much better end
> result.
I looked and didn't see any missing semicolons for NEXTBYTE, but you are 
correct, I think almost every invocation of NEEDBITS and DUMPBITS are 
missing semicolons.
> However, the PKZIP_BUG_WORKAROUND ifdefary hiding braces still throws
> things off, this time in the opposite direction.  NOMEMCPY also gets in
> the way.
>
> I'd be tempted to suggest breaking out a patch earlier dropping these
> two, and then doing the semicolon fixes in this one along with the other
> work.
Upon checking, I did not see any way to set PKZIP_BUG_WORKAROUND and 
NOMEMCPY, so yes I think we can safely drop them.
> Thoughts?
I think it is all doable, the only question is would you prefer to see 
the PKZIP_BUG_WORKAROUND and NOMEMCPY drop happen before relocating the 
files or after relocation?
v/r,
dps