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

["ronnie sahlberg" <ronniesahlberg@xxxxxxxxx>]

I have checked in the wiretap patch after doing the changes listed.


On 3/21/07, Shaun Jackman <sjackman@xxxxxxxxx> wrote:
> 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