https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2935
--- Comment #6 from David Castleford <david.castleford@xxxxxxxxxxxxxxxxxx> 2008-10-07 06:59:38 PDT ---
(In reply to comment #4)
> A few remarks after a quick review:
>
> Replace // comment strings with /* */. The double slash is C++ only.
OK
> Clean up development left overs as well please.
OK
> value_string arrays have to end in {0, NULL} tuple to cover non listed values.
OK, added for messagetypenames, for parametertypenames value 0 is covered by
SIMULCRYPT_DVB_RESERVED = 0x0000
> {name, abbrev, type, display, strings, bitmask, blurb, HFILL}
> if abbrev eq blurb then blurb can be NULL.
OK, used NULL
>
> { &hf_simulcrypt_version,
> { "Version", "simulcrypt.version", FT_UINT8, BASE_HEX, NULL, 0x0,
> "Version Length", HFILL }},
> Is it version or version length ?
good catch, it's version, set to NULL now
>
> FT_BOOLEAN fields have no BASE_, they have a fieldwidth.
OK, 1 byte so set to 8
>
> + tempType = tvb_get_guint8(tvb,1); // Get the type 2 bytes - MSB byte
> + type+=tempType;
> + type=type<<8; // shift 1 byte as MSB
> + tempType = tvb_get_guint8(tvb,2); // Get the type 2 bytes - LSB byte
> + type+=tempType; // add LSB to 2 byte type
> why not use
> + type = tvb_get_ntohs(tvb, 1);
> This comes up numerous times.
thanks, have done so, much nicer code.
>
will reply to other comments, recompile and check things still work, create new
diff file and upload patch
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.