Wireshark · Wireshark-dev: Re: [Wireshark-dev] Bug 8416 - remove C++ incompatibilities from packet-pw-atm.c

[Evan Huus <eapache@xxxxxxxxx>]

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 <jaap.keuter@xxxxxxxxx> 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 <wireshark-dev@xxxxxxxxxxxxx>
> Archives:    http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>              mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe