tags:

views:

99

answers:

4

As apparent in the title, I'm questioning the reason behind defining the macros inside a struct. I frequently see this approach in network programming for instance following snippet:

struct sniff_tcp {
    u_short th_sport;               /* source port */
    u_short th_dport;               /* destination port */
    tcp_seq th_seq;                 /* sequence number */
    tcp_seq th_ack;                 /* acknowledgement number */
    u_char  th_offx2;               /* data offset, rsvd */
#define TH_OFF(th)      (((th)->th_offx2 & 0xf0) >> 4)
    u_char  th_flags;
    #define TH_FIN  0x01
    #define TH_SYN  0x02
    #define TH_RST  0x04
    #define TH_PUSH 0x08
    #define TH_ACK  0x10
    #define TH_URG  0x20
    #define TH_ECE  0x40
    #define TH_CWR  0x80
    #define TH_FLAGS        (TH_FIN|TH_SYN|TH_RST|TH_ACK|TH_URG|TH_ECE|TH_CWR)
    u_short th_win;                 /* window */
    u_short th_sum;                 /* checksum */
    u_short th_urp;                 /* urgent pointer */
};

This example is from sniffex.c code in tcpdump's web site.

Is this for enhancing readability and making code clearer.

+3  A: 

Well, the defined constants were relevant to the possible values of one of the fields.

So, the author decided to improve code locality, and make the API users avoid running in circles. Seems logical.

Otherwise, preprocessor is completely independent of code. You can even put a define inside an expression.

Pavel Radzivilovsky
Ok so it doesn't have any other function. But this looked really weird to me, because for me it's just complicating things. I don't find it meaningful to put something inside a struct that you can't call it within the corresponding struct. For me, if you want code locality you can just put it above the struct.
systemsfault
+1  A: 

i think, this is no "best practice", one should keep the defines (for values) near the struct, but not inside the struct.

(Even better would be to enum and typedef the constants, so a compiler could warn if not typed properly).

The TH_OFF() macro is another case, where it "hides" another element, so maybe it could be put at this position (with an appropriate comment)

Peter Miehle
+1 for enum. I like using an enum for some fields like a state, but I don't think it's appropriate for masked bits in a field.
tomlogic
+2  A: 

We have to suspect that it's attempting to convey to us that the macros are only to be used in conjunction with data in this struct, but it's a poor and convoluted way to represent this.

In C++ it could be preferable to use a nested enum and an inline function to accomplish this, but since the code is C a macro is probably the best alternative.

In my opinion it decreases readability and I'd prefer to see the macros outside of the struct with comments indicating where and how they should be used. That way there's no guessing as to what exactly the macros are for and the struct definition stays free of cruft.

Dan Olson
+1  A: 

The intent of the author seems to be that things that belong together should be located together. Thus the flag macros should be just beneath the flag. With the very same logic, a macro definition has nothing to do with a struct declaration, so the macros do not belong there. There is nothing wrong with putting them just above or beneath the struct.

I wonder if the author would have been consistent and done the same thing if the flag would have been an u_long with 32 flag macros and some more additional default combination macros?

Secure