Huge thanks to our Platinum Members Endace and LiveAction,
and our Silver Member Veeam, for supporting the Wireshark Foundation and project.

Wireshark-bugs: [Wireshark-bugs] [Bug 5749] Improve Connection Manager and Connection Configurat

Date: Thu, 28 Apr 2011 14:36:23 -0700 (PDT)
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5749

--- Comment #9 from Jeff Morriss <jeff.morriss.ws@xxxxxxxxx> 2011-04-28 14:36:21 PDT ---
(In reply to comment #8)
> (In reply to comment #6)
> > Some comments:
> > Why all the proto_tree_add_text()s?  Shouldn't those be filterable fields
> > (added with proto_tree_add_item())?  As it says in README.developer,
> > proto_tree_add_text() should normally be avoided.
> 
> I don't think the intent was to make all of the fields filterable, which is why 
> proto_tree_add_text() was used.

But why?  The filters are what make the decoding really useful. :-)

> > Shouldn't some of this dissection be broken down into subroutines? There are
> > now three 700+ line functions.  Then again, use of add_item() may shorten those
> > functions substantially...  This would also help eliminate the need to indent
> > code that is adding things to a subtree.  That is, you shouldn't have to resort
> > to that to make the code readable.
> > Also: doing:
> > if (tvb_get_guint8(tvb, offset) == XXX)
> > [...]
> > else if (tvb_get_guint8(tvb, offset) == YYY)
> > [...]
> > else if (tvb_get_guint8(tvb_offset) == ZZZ)
> > would be better done with a variable or maybe even another switch statement. 
> > (I suppose the compiler will be smart enough to put it in a variable for you,
> > but it still could be more readable.)
> 
> In my defense, I was keeping with the "coding style" of the dissector.  The
> protocol itself is fairly large (and the current dissector has only scratched
> the surface). I agree with your suggestions, but didn't want to "restyle" the
> dissector, as I thought that would have been more difficult to merge.

True, I can see & appreciate that.  But at some point breaking the mold/style
becomes necessary.

-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.