Hi Ed, I agree with Jaap regarding the replacing of the enum, but that's not your fault, and I suspect the majority of the other enum-conversion cases will not have this issue.
The rest of the patch looks fine to me, but I'm out of time for a proper review+checkin right now. Cheers, Evan On Thu, Feb 28, 2013 at 5:03 PM, Jaap Keuter <[email protected]> wrote: > Hi, > > I already had my reservations against the construction of the PW dissectors, > and > this only adds to it. > > What I would like to see is the abolishment of the pwc_packet_properties_t > type > altogether. This is _not_ an enum. > > typedef enum { > PWC_CW_BAD_BITS03 = 1 << 0 > ,PWC_CW_BAD_PAYLEN_LT_0 = 1 << 1 > ,PWC_CW_BAD_PAYLEN_GT_PACKET = 1 << 2 > ,PWC_CW_BAD_LEN_MUST_BE_0 = 1 << 3 > ,PWC_CW_BAD_FRAG = 1 << 4 > ,PWC_CW_BAD_RSV = 1 << 5 > ,PWC_CW_BAD_FLAGS = 1 << 8 > ,PWC_CW_BAD_PAYLEN_LE_0 = 1 << 9 > ,PWC_CW_BAD_PADDING_NE_0 = 1 << 10 > ,PWC_ANYOF_CW_BAD = PWC_CW_BAD_BITS03 > + PWC_CW_BAD_PAYLEN_LT_0 > + PWC_CW_BAD_PAYLEN_GT_PACKET > + PWC_CW_BAD_LEN_MUST_BE_0 > + PWC_CW_BAD_FRAG > + PWC_CW_BAD_RSV > + PWC_CW_BAD_FLAGS > + PWC_CW_BAD_PAYLEN_LE_0 > + PWC_CW_BAD_PADDING_NE_0 > ,PWC_CW_SUSPECT_LM = 1 << 6 > ,PWC_ANYOF_CW_SUSPECT = PWC_CW_SUSPECT_LM > ,PWC_PAY_SIZE_BAD = 1 << 7 > } > pwc_packet_properties_t; > > It's replacement is a list of #define's or const int's (to stay in C++ realm) > > #define PWC_CW_BAD_BITS03 (1 << 0) > #define PWC_CW_BAD_PAYLEN_LT_0 (1 << 1) > #define PWC_CW_BAD_PAYLEN_GT_PACKET (1 << 2) > #define PWC_CW_BAD_LEN_MUST_BE_0 (1 << 3) > #define PWC_CW_BAD_FRAG (1 << 4) > #define PWC_CW_BAD_RSV (1 << 5) > #define PWC_CW_SUSPECT_LM (1 << 6) > #define PWC_PAY_SIZE_BAD (1 << 7) > #define PWC_CW_BAD_FLAGS (1 << 8) > #define PWC_CW_BAD_PAYLEN_LE_0 (1 << 9) > #define PWC_CW_BAD_PADDING_NE_0 (1 << 10) > #define PWC_ANYOF_CW_BAD (PWC_CW_BAD_BITS03 | > PWC_CW_BAD_PAYLEN_LT_0 | > PWC_CW_BAD_PAYLEN_GT_PACKET | > PWC_CW_BAD_LEN_MUST_BE_0 | > PWC_CW_BAD_FRAG | > PWC_CW_BAD_RSV | > PWC_CW_BAD_FLAGS | > PWC_CW_BAD_PAYLEN_LE_0 | > PWC_CW_BAD_PADDING_NE_0) > #define PWC_ANYOF_CW_SUSPECT PWC_CW_SUSPECT_LM > > > This would impact packet-pw-cesopsn.c, packet-pw-satop.c in a similar way as > it > did packet-pw-atm.c > > Thanks, > Jaap > > > On 02/28/2013 09:57 PM, Ed Beroset wrote: >> As mentioned in the subject line, I've added Bug 8416 - "remove C++ >> incompatibilities from packet-pw-atm.c" with the associated patch. Doing a >> little forensic work on the C++ incompatibilities still present in the code >> base, here are the types of issues of the 4919 "c++-incompat" lines in a >> compilation of the latest source using gcc on a Linux box (Fedora 17) >> (before this patch): >> >> type count percent >> implicit_casts 4013 81.58% >> keyword_use 634 12.89% >> enum_conversion 197 4.00% >> uninit_const 7 0.14% >> field_typedef 5 0.10% >> special_operator 3 0.06% >> incompat_ptr 2 0.04% >> other 58 1.18% >> >> It's clear that the vast majority of these (over 98%) are of only three >> different kinds which are mostly trivial fixes. I do want to point out, >> however, that the way I chose to resolve the enum_conversion complaint was >> to change the type of one member of a struct from an enum type to an int. >> The longer version of the rationale is in the bug report. If we find this >> kind of patch acceptable, and desirable, I'll do more. >> >> https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=8416 >> >> Ed >> > > ___________________________________________________________________________ > Sent via: Wireshark-dev mailing list <[email protected]> > Archives: http://www.wireshark.org/lists/wireshark-dev > Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev > mailto:[email protected]?subject=unsubscribe ___________________________________________________________________________ Sent via: Wireshark-dev mailing list <[email protected]> Archives: http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:[email protected]?subject=unsubscribe
