https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5222
Jeff Morriss <jeff.morriss.ws@xxxxxxxxx> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jeff.morriss.ws@xxxxxxxxx
--- Comment #6 from Jeff Morriss <jeff.morriss.ws@xxxxxxxxx> 2010-10-10 18:45:44 PDT ---
A couple of comments:
- No C++/C99 style comments please
- be sure all your value_strings are NULL terminated (I saw at least one that
wasn't).
- I suspect the new call to tvb_length() should be tvb_reported_length() (this
matters when the packets are cut short due to snapshot length)
- All of these things "+ case 0x03: /* secure */" would be better off as
macros (e.g., #define AMQP SECURE 0x03). These could also then be used in the
corresponding value_strings. The same goes for a lot of the numbers sprinkled
throughout, though the commented ones are the most obvious area for
improvement.
- DISSECTOR_ASSERT() should only be used for detecting bugs in the
dissector--not malformed packets. Add an expert info instead or if you really
want to raise a flag (and stop dissection) throw a ReportedBoundsError. (I see
that the original dissector used such assertions a lot improperly, but we're
trying to get rid of those--and not let more in.)
- the proto_tree_add_uint64() call can/should be proto_tree_add_item()
- A few of the blurbs (FIELDDESCR) in the new hf's are redundant with the
name--use NULL instead in this case. (tools/checkAPIs.pl now detects these for
you.)
Overall it looks quite good--the above are mostly nits or things we can fix
while checking it in, except for the DISSECTOR_ASSERT() and C++-style comments
issues.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.