Comment # 13
on bug 8431
from Steve Magnani
I took a quick look. Here are some comments:
packet-usb-ptp.h:
- I wasn't expecting to see array declarations in a .h file
- hf_ prefixes are usually reserved for field handles -
it's confusing to use them for string arrays
- "libghoto2" should be "lipgphoto2"?
packet-usb-ptp.c:
- Functions except the last 2 should all be 'static'
- usb_ptp_flavor(): consider implementing as switch
- dissect_usb_ptp_params():
'while (remaining && remaining >= 4)': can remove 'remaining &&'
- dissect_usb_ptp():
* remove _U_ notation from ptp_length
* Difficult to read offset increment statements on same line as other code
* Consider expanding "col_class" to make the Info column display a little
more intuitive for novice users, i.e. "CMD/RSP/DAT/EVT" instead of
"C/R/D/E"
- proto_register_usb_ptp()
* hf[]: Use VALS() for the 'strings' field
In general I think more whitespace would make the code easier to review, i.e.:
offset=usb_ptp_add_array_is(pinfo,tree,hf_devinfo_operationsupported ...
-->
offset = usb_ptp_add_array_is(pinfo, tree, hf_devinfo_operationsupported ...
You are receiving this mail because:
- You are watching all bug changes.