https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5749
Joakim Wiberg <jow@xxxxxx> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jow@xxxxxx
--- Comment #11 from Joakim Wiberg <jow@xxxxxx> 2011-05-03 05:59:44 PDT ---
(In reply to comment #9)
> (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. :-)
>
It certainly makes send to add more filters.
> > > 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.
This dissector for sure needs to be clean up, it will make it a lot easier to
maintain the code. But wouldn't it make more sense to do this as a separate
patch, just including the cleanup to follow the coding style? I could volunteer
doing this after Mikes patch has been committed.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.