[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5051

--- Comment #6 from Bill Meier <wmeier@xxxxxxxxxxx> 2010-08-06 09:54:23 EDT ---
OK: I've looked at the patch in more detail:

My last (I think) comment on the patch:

To be honest, I'm not really thrilled by the use of /* ... */ to comment out
parts of the code:

Eg:

/* if (bacapp_seq > 0) /* Can't handle continuation segments, so just treat as
data */
/* {
      proto_tree_add_text(bacapp_tree, tvb, offset, 0, "(continuation)");
      return offset;
   }
   else
   { */
   /* Service ACK follows... */
      return fConfirmedServiceAck (tvb, pinfo, bacapp_tree, offset, svc);
/* } */
}

I find the above rather unreadable (not to mention that the gcc compiler will
produce a warning message: "/*" within comment).

Another example:

    frag_msg = fragment_add_seq_check(tvb, data_offset, pinfo,
/*  frag_msg = fragment_add_seq_check(tvb, bacapp_seqno == 0 ? 0 : data_offset,
pinfo, */
                   bacapp_invoke_id, /* ID for fragments belonging together */
                   msg_fragment_table, /* list of message fragments */
                   msg_reassembled_table, /* list of reassembled messages */
                   bacapp_seqno, /* fragment sequence number */
                   tvb_reported_length_remaining(tvb, data_offset), /* fragment
length - to the end */
/*                 tvb_reported_length_remaining(tvb, bacapp_seqno == 0 ? 0 :
data_offset),  fragment length - to the end */

This is somewhat more readable but still messy.

In both cases (and similar) if you feel that the original code should remain as
a comment, may I suggest:

#if 0
   <orig code>
#endif
   <replacement/new code>

#if 0
if (bacapp_seq > 0) /* Can't handle continuation segments, so just treat as
data */
   {
      proto_tree_add_text(bacapp_tree, tvb, offset, 0, "(continuation)");
      return offset;
   }
   else
   {
   /* Service ACK follows... */
      return fConfirmedServiceAck (tvb, pinfo, bacapp_tree, offset, svc);
   }
#endif
   /* Service ACK follows... */
   return fConfirmedServiceAck (tvb, pinfo, bacapp_tree, offset, svc);
}

IMHO it's not really necessary in many cases to leave the old code. One can
always look at the svn diff if 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.