Pascal Quantin
changed
bug 9047
Comment # 10
on bug 9047
from Pascal Quantin
Hi Roland,
thanks for your patch. You will find a few comments below.
Changing all the indentation from 4 spaces to tab makes the patch really huge
without any real reason (it defeats a svn blame for example for a quick check
to easily find the reason why a code line was changed). Why not keep the
existing indentation?
Moreover I would have preferred a patch removing the tree checks in existing
code separated from the patch adding new code, but that's also a matter of
taste.
Regarding this hunk:
item = proto_tree_add_uint_format_value(epl_tree, hf_epl_asnd_svid, tvb,
offset, 1,
svid, "%s (%d)",
val_to_str_const(svid, asnd_svid_vals, "Unknown"),
svid);
Why not simply use proto_tree_add_uint?
Regarding hf_epl_asnd_sdo_cmd_data_data, why not use FT_BYTES instead of
FT_NONE and replace all the if/else if blocks by a simple
proto_tree_item(sdo_data_tree, hf_epl_asnd_sdo_cmd_data_data, tvb, offset,
size,
ENC_NA);
The 'tvb_reported_length_remaining != 0' test should be replaced by
'tvb_reported_length_remaining > 0' as it can return -1
The initialization of most (all?) variables at the beginning of
dissect_epl_sdo_command_write_multiple_by_index() function seems useless.
In the following hunk:
if ( offsetincrement <= 0 )
{
datalength = tvb_reported_length_remaining ( tvb, offset );
size = tvb_reported_length_remaining ( tvb, offset ) - (
pyldoffset );
lastentry = TRUE;
}
How can offsetincrement be negative? (it is defined as guint32)
Best regards,
Pascal.
You are receiving this mail because:
- You are watching all bug changes.
