Wireshark · Wireshark-dev: Re: [Wireshark-dev] [PATCH 1/2] wiretap: New MPEG file format

["Shaun Jackman" <sjackman@xxxxxxxxx>]

On 3/20/07, ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote:
> 1, shouldnt the defines MPA_MARSHAL_... really be called
> MPA_UNMARSHAL_... instead?

Good point.

> 2, do you really need all these includes?
> +#include <arpa/inet.h>
> +#include <errno.h>
> +#include <math.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <time.h>
> are all of these ones actually available on all platforms we support?

At one point each of these includes was required. I'll double check to
make sure they are all still relevant. They are all very standard
includes. I would be quite surprised if a platform didn't include
them. It certainly wouldn't be a POSIX platform.

> 3, not all compilers we support allow you to declare variables mid-block
> +               if (file_seek(wth->fh, 3, SEEK_CUR, err) == -1)
> +                       return FALSE;
> +
> +               uint8_t stream;
>
> 4, dont use these non-guintX types
> uint8_t
> use the g[u]int{8|16|32} from glib instead of these _t types.

I prefer the standard uint8_t types from the Single UNIX Specification
(SUSv3) and probably other major standards over some local flavour of
guint8. It makes the code more readable and portable. I *really* don't
see the point to prefixing standard symbols with the letter g because
somebody decided to reinvent the wheel.

I'm going on vacation tomorrow for two months or so. If the above
matters are the only thing standing in between checking in this patch,
could the maintainer applying the patch to the repository possibly fix
the couple minor issues before checking in? If not, I'll look at it
again once I return.

Cheers,
Shaun