On Fri, May 12, 2000 at 12:30:59PM -0500, Ben Fowler wrote:
>
>
>
> The enclosed patches changes do the following:
>
> 1) Add a number of assertion statements to packet.c.
You added one to packet.c. That's okay.
You added many to proto.c. I'd rather just keep the
g_assert() in proto_registrar_get_nth() and add the g_assert(
length > 0) to proto_tree_add_field_info(). I know you've
said before that you want to assert as soon as possible
(http://ethereal.zing.org/lists/ethereal-dev/200004/msg00229.html) but
it really does make sense, in this case, to keep the asserts in the few
central functions that are always called no matter which path the
code takes.
Reasons:
1. Why assert on hfindex in each of the proto_tree_add_*() calls?
They don't actually use hfindex, they just pass the value to
proto_registrar_get_nth(). Let's let the function that _uses_
the value make assertions about the validity of the data.
(of course, I'm probably embarassing myself as I may have written
code that violates this premise. :-)
2. Even through g_assert() prints the line number where
the assertion occurs, you'll still want to do a backtrace in your
debugger to see who called the proto_tree_add_*() function. In that
case, it doesn't matter where in the chain the assert is, because
you'll still get the important information about which function
sent the erroneous data only through the debugger.
> 2) Add a symbol BASE_MAX (to use in assertion statements,
> 3) Include stdio.h in tvbuff.c (to support fprintf( ) )
Why? I don't see fprintf() being used. Is some macro expanding
to fprintf()?
> 4) Change in white space. Where do you like spaces and parentheses?
> I put 1 space after each opening parenthesis and before each closing
> parenthesis.
>
> Apologies if I have inadvertently included patches which have
> already been rejected.
>
> Is it best to put related patches, grouped in separate files, or
> are you happy to have them in one.
Unrelated modification should be sent in separate patch files, so that
decisions can be made about them independent of each other.
--gilbert
