Thread: Looking for an overview from the Linux experts.

  1. #1
    Registered User
    Join Date
    Mar 2012
    Location
    the c - side
    Posts
    373

    Looking for an overview from the Linux experts.

    Could the Linux experts here give my program here a check, and
    maybe offer some constructive criticism.

    I've used sockets to drive my c programming forward, and
    I think this is a good representation of where my coding
    ability in c is currently.

    It would be good to hear some opinions from more experienced
    programmers.

    The speed of checksum calculation isn't a concern in a ping program,
    so I've taken the opportunity to use my own checksum function,
    which means I'm completely to blame for the code.

    (The Windows Version is closed source. )

    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    #include <time.h>
    #include <unistd.h>
    #include <sys/types.h>
    #include <sys/socket.h>
    #include <sys/time.h>
    #include <arpa/inet.h>
    #include <netinet/ip.h>
    #include <netinet/tcp.h>
    #include <netinet/ip_icmp.h>
    #include <stdint.h>
    #include <errno.h>
    #include <netdb.h>
    
    /********************************************/
    //
    //          Basic ping program.
    //
    //
    //
    //
    //          gemera        20th Aug 2013
    //
    /********************************************/
    
    #define MAX_DATAGRAM 225                // max size of datagrams
    #define DEFAULT_PINGS 5                 // default number of pings to send
    #define IDENTIFIER 31137                // arbitrary default identifier for icmp messages
    #define MAX_IDENTIFIER 65535            // max identifier value
    #define ICMP_ECHO_REPLY 0               // icmp type identifier for echo reply
    #define DESTINATION_UNREACHABLE 3       // icmp type identifier for destination unreachable error
    #define ICMP_ECHO_REQUEST 8             // icmp type identifier for echo request
    #define TIME_EXCEEDED 11                // icmp type identifier for time exceeded error
    #define MAX_TTL 255                     // max ttl value
    #define MAX_EXTRA_ARG_PAIRS 3            // max number of extra argument pairs
    #define CARRY_BIT 0x10000                // used in checksum function
    
    #define TRUE 1
    #define FALSE 0
    
    struct icmp_options             // own struct for icmp options to insert send time
    {
        int32_t time_sec;
        int32_t time_usec;
    };
    
    static void check_command_line(const int argc,const char* const* const argv,int* ttl,unsigned* identifier,unsigned* no_of_pings);
    static void init_addr_struct(struct sockaddr_in* target,const char* const* const argv);
    static int get_socket(const int ttl_value);
    static int send_echo_request(const int raw_socket,char* datagram,const struct sockaddr_in target,struct timeval time_start,const unsigned seq,const int ttl,const unsigned short int identifier);
    static int recv_echo_reply(const int raw_socket,char* datagram,struct sockaddr_in* receive,struct timeval* time_end);
    static void display_results(const char* datagram,struct timeval time_start,const struct timeval time_end,const struct sockaddr_in receive);
    static unsigned short check_sum (const unsigned short* header, const int word_count);
    
    int main(int argc, char** argv)
    {
        char datagram[MAX_DATAGRAM];            // buffer for datagram
        struct sockaddr_in target = {0};        // address structure for sendto
        struct sockaddr_in receive = {0};       // address structure for recvfrom
        int raw_socket;
        unsigned int pings;
        unsigned int no_of_pings = DEFAULT_PINGS;
        unsigned identifier = IDENTIFIER;
        int ttl = MAX_TTL;
        struct timeval time_start = {0};        // to store send time
        struct timeval time_end = {0} ;            // to store recv time
        int recv_err_flag;                        // error flag for received messages
        int sent_err_flag;                        // error flag for sent messages
        int sent_count = 0;                        // count of sent messages
        int recv_count = 0;                        // count of received messages
    
        check_command_line(argc,(const char* const* const)argv,&ttl,&identifier,&no_of_pings);   // check command line arguments
        init_addr_struct(&target,(const char* const* const)argv);                                 // initialise target address struct
        raw_socket = get_socket(ttl);                                                           // initialise and get raw socket
    
        printf("\n\n");
    
        for(pings = 0;pings < no_of_pings;pings++)
        {
            sent_err_flag = FALSE;
            recv_err_flag = FALSE;
    
            sent_err_flag = send_echo_request(raw_socket,datagram,target,time_start,pings,ttl,identifier);  // send icmp echo request
            if(sent_err_flag == FALSE)
            {
                sent_count++;
            }
    
            recv_err_flag = recv_echo_reply(raw_socket,datagram,&receive,&time_end);             // receive echo reply
            if(recv_err_flag == FALSE)
            {
                display_results(datagram,time_start,time_end,receive);
                recv_count++;
            }
    
            sleep(1);
        }
    
        printf("\n\n\t Packets Sent: %d  Packets Received: %d  Packets Lost: %d\n\n", sent_count,recv_count,sent_count - recv_count);
    
        close(raw_socket);
        return 0;
    }
    
    static void check_command_line(const int argc,const char* const* const argv,int* ttl,unsigned* identifier,unsigned* no_of_pings)
    {
        const char* const cmd_prefix[] = {"-t","-i","-n"};
        int found_flag[] = {FALSE,FALSE,FALSE};
        int i,j;
    
        if(argc < 2 || argc > 8 || (argc % 2 != 0))
        {
            fprintf(stderr," \n Usage: %s <target host or ip> [-t ttl value] [-i identifier] [-n no.of pings]\n",argv[0]);
            exit(EXIT_FAILURE);
        }
    
        if(argc == 2)
        {
            return;
        }
    
        for(i = 3;i <= argc;i = i + 2)                               // deal with extra arguments in pairs
        {
            for(j = 0;j < MAX_EXTRA_ARG_PAIRS;j++)                     // 3 command prefix to search for
            {
                if((strcmp(argv[i - 1],cmd_prefix[j]) == 0))
                {
                    if(j == 0 && found_flag[j] == FALSE)
                    {
                        if((*ttl = atoi(argv[i])) > 0 && *ttl <= MAX_TTL)
                        {
                            found_flag[j] = TRUE;
                        }
                        else
                        {
                            fprintf(stderr,"\n Error: ttl range is 1 to %d.\n",MAX_TTL);
                            exit(EXIT_FAILURE);
                        }
                    }
                    else if(j == 0 && found_flag[j] == TRUE)
                    {
                        fprintf(stderr,"\n Error: option has been duplicated.\n");
                        exit(EXIT_FAILURE);
                    }
    
                    if(j == 1 && found_flag[j] == FALSE)
                    {
                        if((*identifier = atoi(argv[i])) >= 0 && *identifier <= MAX_IDENTIFIER)
                        {
                            found_flag[j] = TRUE;
                        }
                        else
                        {
                            fprintf(stderr,"\n Error: identifier range is 0 to %d.\n",MAX_IDENTIFIER);
                            exit(EXIT_FAILURE);
                        }
                    }
                    else if(j == 1 && found_flag[j] == TRUE)
                    {
                        fprintf(stderr,"\n Error: option has been duplicated.\n");
                        exit(EXIT_FAILURE);
                    }
    
                    if(j == 2 && found_flag[j] == FALSE)
                    {
                        if((*no_of_pings = atoi(argv[i])) > 0)
                        {
                            found_flag[j] = TRUE;
                        }
                        else
                        {
                            fprintf(stderr,"\n Error: number of pings must be greater than zero.\n");
                            exit(EXIT_FAILURE);
                        }
                    }
                    else if(j == 2 && found_flag[j] == TRUE)
                    {
                        fprintf(stderr,"\n Error: option has been duplicated.\n");
                        exit(EXIT_FAILURE);
                    }
                }
            }
        }
    }
    
    static void init_addr_struct(struct sockaddr_in* target,const char* const* const argv)
    {
        struct hostent* host;                           // kernel takes care of ports
                                                        // and of source sockaddr_in
        target->sin_family = AF_INET;
    
        if((target->sin_addr.s_addr = inet_addr(argv[1])) == INADDR_NONE)        // check if target is ip or host name
        {
            if((host = gethostbyname(argv[1])) != NULL)
            {
                memcpy(&(target->sin_addr.s_addr),host->h_addr,host->h_length);
            }
            else
            {
                fprintf(stderr,"\n Error: invalid host name.\n");
                exit(EXIT_FAILURE);
            }
        }
    }
    
    static int get_socket(const int ttl_value)
    {
        int ret_val;
        int raw_socket;
        struct timeval time_wait;
    
        time_wait.tv_sec = 1;                        // 1 second time wait
        time_wait.tv_usec = 0;
    
        raw_socket = socket(PF_INET,SOCK_RAW,IPPROTO_ICMP);
        if(raw_socket == -1)
        {
            perror(" Raw Socket Error");
            exit(EXIT_FAILURE);
        }
    
        ret_val = setsockopt(raw_socket,SOL_SOCKET,SO_SNDTIMEO,(void*)&time_wait,sizeof(time_wait));
        if(ret_val == -1)
        {                                            // set 1s blocking time on send
            perror(" SO_SNDTIMEO Error");
            exit(EXIT_FAILURE);
        }
    
        ret_val = setsockopt(raw_socket,SOL_SOCKET,SO_RCVTIMEO,(void*)&time_wait,sizeof(time_wait));
        if(ret_val == -1)
        {                                            // set 1s blocking time on recv
            perror(" SO_RCVTIMEO Error");
            exit(EXIT_FAILURE);
        }
    
        ret_val = setsockopt(raw_socket,IPPROTO_IP,IP_TTL,(char*)&ttl_value,sizeof(ttl_value));
        if(ret_val == -1)
        {                                            // set ip header ttl value
            perror(" IP_TTL Error");
            fprintf(stderr,"\n Unable to set IP_TTL\n");
            exit(EXIT_FAILURE);
        }
    
        return raw_socket;
    }
    
    static int send_echo_request(const int raw_socket,char* datagram,const struct sockaddr_in target,struct timeval time_start,const unsigned int seqs,const int ttl,const unsigned short int identifier)
    {
        struct icmphdr* icmp_ptr;
        struct icmp_options* option_ptr;
        int ret_val;
        int err_flag = FALSE;
    
        gettimeofday(&time_start,NULL);     // save time datagram sent
    
        memset(datagram,0,MAX_DATAGRAM);
    
        icmp_ptr = (struct icmphdr*)datagram;
        option_ptr = (struct icmp_options*)(datagram + sizeof(struct icmphdr));
    
        option_ptr->time_sec = htonl(time_start.tv_sec);
        option_ptr->time_usec = htonl(time_start.tv_usec);
    
        icmp_ptr->type = ICMP_ECHO_REQUEST;
        icmp_ptr->code = 0;
        icmp_ptr->un.echo.id = htons(identifier);
        icmp_ptr->un.echo.sequence = htons(seqs);
        icmp_ptr->checksum = 0;
        icmp_ptr->checksum = check_sum((unsigned short const* const)datagram ,(sizeof(struct icmphdr) + sizeof(struct icmp_options))/2);
    
        ret_val = sendto(raw_socket,datagram,sizeof(struct icmphdr) + sizeof(struct icmp_options),0,(struct sockaddr*)&target,sizeof(target));
        if(ret_val == -1)
        {
            err_flag = TRUE;
    
            if(errno == EWOULDBLOCK)
            {
                fprintf(stderr,"\n Error: sendto timed out.\n");
            }
            else
            {
                perror(" sendto() error");
            }
        }
    
        return err_flag;
    }
    
    static int recv_echo_reply(const int raw_socket,char* datagram,struct sockaddr_in* receive,struct timeval* time_end)
    {
        int err_flag = FALSE;
        int ret_val;
        unsigned int sze = sizeof(struct sockaddr_in);
    
        memset(datagram,0,MAX_DATAGRAM);
    
        ret_val = recvfrom(raw_socket,datagram,MAX_DATAGRAM,0,(struct sockaddr*)receive,&sze);
        if(ret_val == -1)
        {
            if(errno == EWOULDBLOCK)
            {
                fprintf(stderr,"\n Error: recvfrom timed out.\n");
            }
            else
            {
                 perror(" recvfrom() error");
            }
    
            err_flag = TRUE;
        }
    
        gettimeofday(time_end,NULL);     // save time message received
    
        return err_flag;
    }
    
    static void display_results(const char* datagram,struct timeval time_start,const struct timeval time_end,const struct sockaddr_in receive)
    {
        struct icmphdr* icmp_ptr;
        struct icmp_options* option_ptr;
        struct iphdr* ip_head_ptr;
        unsigned char ip_length;
        int message_length;
        long diff_usecs;
        long diff_secs;
        double round_trip_time;
        const char* const icmp_msg[] = {"Network Unreachable.","Host Unreachable.","Protocol Unreachable.","Port Unreachable.","Fragmentation Needed but DF bit set.",
                            "Source Route Failed.","Destination Network Unknown.","Destination Host Unknown.","Source Host Isolated",
                            "Destination Network Administratively Prohibited.","Destination Host Administratively Prohibited.",
                            "Network Unreachable For TOS.","Host Unreachable For TOS. ","Communications Administratively Prohibited.",
                            "Host Precedence Violation.","Precedence Cutoff In Effect."};
    
        ip_head_ptr = (struct iphdr*)datagram;     // check size of ip header
        ip_length = (ip_head_ptr->ihl) * 4;
    
        icmp_ptr = (struct icmphdr*)(datagram + ip_length);
        option_ptr = (struct icmp_options*)(datagram + ip_length + sizeof(struct icmphdr));
    
        if(icmp_ptr->type == ICMP_ECHO_REPLY)
        {
            message_length = sizeof(struct icmphdr) + sizeof(struct icmp_options);
    
            time_start.tv_sec  = ntohl(option_ptr->time_sec);                 // get delta time
            time_start.tv_usec = ntohl(option_ptr->time_usec);
    
            diff_secs = time_end.tv_sec - time_start.tv_sec;
            diff_usecs = time_end.tv_usec - time_start.tv_usec;
            diff_usecs = diff_secs*1000000 + diff_usecs;
    
            round_trip_time = (double)diff_usecs/1000;                         // time in ms
    
            printf(" %d bytes from %s id = %d: seq = %d ttl = %d rtt = %.3lf ms\n",message_length + ip_length,inet_ntoa(receive.sin_addr),ntohs(icmp_ptr->un.echo.id),ntohs(icmp_ptr->un.echo.sequence),ip_head_ptr->ttl,round_trip_time);
        }
        else if(icmp_ptr->type == DESTINATION_UNREACHABLE)
        {
            printf(" from %s : %s ttl = %d\n",inet_ntoa(receive.sin_addr),icmp_msg[icmp_ptr->code],ip_head_ptr->ttl);
        }
        else if(icmp_ptr->type == TIME_EXCEEDED)
        {
            if(icmp_ptr->code == 0)
            {
                printf(" from %s : Time Exceeded.TTL equals 0 during transit. ttl = %d\n",inet_ntoa(receive.sin_addr),ip_head_ptr->ttl);
            }
            else if(icmp_ptr->code == 1)
            {
                printf(" from %s : Time Exceeded.TTL equals 0 during reassembly. ttl = %d\n",inet_ntoa(receive.sin_addr),ip_head_ptr->ttl);
            }
            else
            {
                 printf(" from %s : Time Exceeded. ttl = %d\n",inet_ntoa(receive.sin_addr),ip_head_ptr->ttl);
            }
        }
        else if(icmp_ptr->type == ICMP_ECHO_REQUEST)
        {
            printf(" from %s : Icmp Echo Request. ttl = %d\n",inet_ntoa(receive.sin_addr),ip_head_ptr->ttl);
        }
        else
        {
            printf(" from %s : Icmp Received. code = %u ttl = %d\n",inet_ntoa(receive.sin_addr),icmp_ptr->code,ip_head_ptr->ttl);
        }
    }
    
    static unsigned short check_sum(const unsigned short* const header, const int word_count)
    {
        int i;
        uint32_t sum = 0;
    
        for(i = 0;i < word_count;i++)
        {
            sum += header[i];
    
            if(sum & CARRY_BIT)
            {
                sum = (sum ^ CARRY_BIT) + 1;
            }
        }
    
        return (unsigned short) ~sum;
    }

  2. #2
    Registered User
    Join Date
    Mar 2012
    Location
    the c - side
    Posts
    373
    The one omission I've spotted is there is no priviledge raising n lowering around the raw socket. That's because the Linux puppy I'm using has root as default - It was ok for beginning in Linux, but I should really be moving completely onto another distro like Ubuntu.

  3. #3
    Registered User
    Join Date
    Jul 2012
    Posts
    51
    Quote Originally Posted by gemera View Post
    Could the Linux experts here give my program here a check, and maybe offer some constructive criticism.
    I'm not an "expert" but...

    Have you looked at other ping implementations (code) to see where you might want or need to improve? The Ping Page

  4. #4
    Registered User
    Join Date
    Mar 2012
    Location
    the c - side
    Posts
    373
    It really wasn't about the fact it's a ping program or even about highlighting specific mistakes (a few), but about checking the overall coding style before its replicated time and again in future programs.

  5. #5
    - - - - - - - - oogabooga's Avatar
    Join Date
    Jan 2008
    Posts
    2,808
    You should pick a maximum line length.
    230 characters is way too long.
    80 or less is/was common.
    Surely 120 is enough for anyone.
    The cost of software maintenance increases with the square of the programmer's creativity. - Robert D. Bliss

  6. #6
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    I only had time for a quick glance, so this is fairly superficial, but overall your program looks good. A few notes.

    I'm with oogabooga on the line length. Since this seems *nix specific, I would lean more toward 80 chars per line.

    A few more comments might be nice, just to give an overview of what your code is doing, especially any parts that might not be intuitive to somebody else look at this or to yourself when you come back to modify it in a month or a year.

    Also, you should look at the getopt/getopt_long/getopt_long_only functions for command-line arg processing, it will greatly clean up check_command_line. A more thorough usage message or --help option would also be good, where you detail what each param is for, it's limits, possible values, etc.

    Lastly, and this is pretty minor: you put a new line before and after each error message. That causes an extra blank line between messages, which will ensure they're on their own line when they're dumped to the screen, but will probably be annoying if you pipe stderr to a log file or pager (like less) for reading.

  7. #7
    Registered User
    Join Date
    Mar 2012
    Location
    the c - side
    Posts
    373
    Ok, given me some food for thought, cheers.

  8. #8
    Registered User
    Join Date
    Mar 2012
    Location
    the c - side
    Posts
    373
    Can I ask what you think of the sizes of the check_cmd_line() (as is) and display_results() functions, which are relatively large. Would there be any merit in functionalising them further?

  9. #9
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    Quote Originally Posted by gemera View Post
    Can I ask what you think of the sizes of the check_cmd_line() (as is) and display_results() functions, which are relatively large. Would there be any merit in functionalising them further?
    This is something of a style/personal preference thing, so this is all just suggestions. You will eventually find what works best for you, and (should you work on team projects), what works best for your team. You will also, as you become more experienced, develop a much better sense of exactly where and how to split up large chunks of code.

    As a rule of thumb, I use "one screenful" as a rough size guide for my functions. By "screenful", I don't mean an old 25-line console, I mean a modern screen (perhaps 50-75 lines long). Much beyond that, and I might break it up into smaller functions just to keep things readable. Another consideration is cyclomatic complexity, i.e. how many paths through the code. If you have a short, but cyclomatically very complex function, it may be worth splitting up.

    check_command_line is not too long (79 lines including blanks, etc), but the cyclomatic complexity is bad (the triple-nested if inside the double-nested loop is a bit too much, IMO). It probably wont shorten the function any, but I would still suggest you use getopt/getopt_long. I would also bet that there are some small bugs in your option handling that a user would somehow manage to stumble on. getopt/getopt_long, however, are very widely used and well-tested. I would not "functionalize" this more, since all the code in there is closely related, and there's no real repeated sections to pull into it's own function.

    As for display_results, at 64 lines, I probably wouldn't break that up either. The cyclomatic complexity is also not too bad. However, if you did break it up, I would do so at the outer 'if' statements, i.e. make each different icmp_ptr->type call a function for displaying that type of results.

  10. #10
    Registered User
    Join Date
    Jun 2005
    Posts
    6,815
    Associated with anduril's concern about cyclomatic complexity in check_command_line(), is the fact that the loop involving the variable j is completely unnecessary. The code within that loop checks the values of j against particular values.

    There is a lot of duplicated code or code that is very similar (either because of copy/paste, or by typing in things repetitively). A small number of helper functions would easily reduce the duplication.

    Several if/else constructs are sequentially checking a variable (or a struct member) against constant values. A switch statement would probably be cleaner (and at least as effective, performance wise).

    You have a lot of code similar to this (sample from lines 130-140)
    Code:
    if((*ttl = atoi(argv[i])) > 0 && *ttl <= MAX_TTL)
    {
         found_flag[j] = TRUE;
    }
    else
    {
         fprintf(stderr,"\n Error: ttl range is 1 to %d.\n",MAX_TTL);
         exit(EXIT_FAILURE);
    }
    which could be trivially simplified to
    Code:
    found_flag[j] = ((*ttl = atoi(argv[i])) > 0 && *ttl <= MAX_TTL);
    if (!found_flag[j])
    {
         fprintf(stderr,"\n Error: ttl range is 1 to %d.\n",MAX_TTL);
         exit(EXIT_FAILURE);
    }
    (and, yes, this is in a section where the variable j always has a value of zero).

    I wouldn't bother with the macros TRUE (expands to 1) and FALSE (expands to zero). Instead of
    Code:
        if (something == TRUE && something_else == FALSE)
            some_flag = TRUE;
    it is easier to type (and also arguably more clear) by doing this as;
    Code:
        some_flag = (something && !something_else);
    Does exactly the same thing, and expresses things as boolean (logical) expressions rather than relying on program-defined magic values.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

  11. #11
    Ticked and off
    Join Date
    Oct 2011
    Location
    La-la land
    Posts
    1,728
    I agree with grumpy, except for a small nitpick.

    The example grumpy showed,
    Code:
        some_flag = (something && !something_else);
    is really equivalent to
    Code:
        if (something == TRUE && something_else == FALSE)
            some_flag = TRUE;
        else
            some_flag = FALSE;
    because some_flag is always set to the result of the comparison.


    The equivalent of
    Code:
        if (something == TRUE && something_else == FALSE)
            some_flag = TRUE;
    is actually (in C)
    Code:
        some_flag |= (something && !something_else);
    or (in general, perhaps easier to read)
    Code:
        some_flag = some_flag || (something && !something_else);

    The equivalent of
    Code:
        if (something == TRUE && something_else == FALSE)
            some_flag = FALSE;
    is actually (in C)
    Code:
        some_flag &= !(something && !something_else);
    or (more generally, perhaps easier to understand)
    Code:
        some_flag = some_flag && !(something && !something_else);
    or equivalent
    Code:
        some_flag = some_flag && (!something || something_else);
    For the cases where a trigger condition sets a flag variable to logical FALSE, I believe the last format is the easiest to use and understand -- read it as "some_flag stays true as long as (not something) or (something_else)".

  12. #12
    Registered User
    Join Date
    Jun 2005
    Posts
    6,815
    Quote Originally Posted by Nominal Animal View Post
    I agree with grumpy, except for a small nitpick.

    The example grumpy showed,
    Code:
        some_flag = (something && !something_else);
    is really equivalent to
    Code:
        if (something == TRUE && something_else == FALSE)
            some_flag = TRUE;
        else
            some_flag = FALSE;
    because some_flag is always set to the result of the comparison.
    It is a very small nitpick, particularly as every time this is done in the OP's code, the program exits if the condition being tested is false.

    Quote Originally Posted by Nominal Animal View Post
    The equivalent of
    Code:
        if (something == TRUE && something_else == FALSE)
            some_flag = TRUE;
    is actually (in C)
    Code:
        some_flag |= (something && !something_else);
    Not (necessarily) true if some_flag, something, or something_else are signed values. Or if some_flag has a value which is any value other than 1 or zero.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

  13. #13
    Ticked and off
    Join Date
    Oct 2011
    Location
    La-la land
    Posts
    1,728
    Quote Originally Posted by grumpy View Post
    It is a very small nitpick
    Absolutely. I wasn't sure if it was worth mentioning, really.

    Quote Originally Posted by grumpy View Post
    Not (necessarily) true if some_flag, something, or something_else are signed values.
    Right, I assumed some_flag, something, and something_else are all logical values there.

    One can use the not operator to ensure logical values, though.
    Code:
    int someflag;
    someflag = !something;
    is equivalent to
    Code:
    int someflag;
    if (something)
        someflag = 0; /* FALSE */
    else
        someflag = 1; /* TRUE */
    Some find it surprising that you can use two not operators to enforce a logical value. In C, this means that
    Code:
    int someflag;
    someflag = !!something;
    is equivalent to
    Code:
    int someflag;
    if (something)
        someflag = 1; /* TRUE */
    else
        someflag = 0; /* FALSE */

    There is a related trick in C that relies on int being twos complement is to substract one from the logical value, turning FALSE to all bits set, and TRUE to all bits zero. (All current widely-used processor architectures use twos complement format for signed integers.) For example,
    Code:
    /* int value; */
    value = value & ((int)(!something) - 1);
    is the equivalent of
    Code:
    if (!something)
        value = 0;
    Using not-not (!!) instead,
    Code:
    /* int value; */
    value = value & ((int)(!!something) - 1);
    is the equivalent of
    Code:
    if (something)
       value = 0;
    I don't recommend using such tricks (except for some limited cases where the compiler really needs such help to avoid a jump or other expensive operation), but I still see such expressions every now and then in code, so I thought it might be helpful to know.

  14. #14
    Registered User
    Join Date
    Jun 2005
    Posts
    6,815
    Quote Originally Posted by Nominal Animal View Post
    Right, I assumed some_flag, something, and something_else are all logical values there.
    Which makes you still incorrect. It is the type that matters when you do bitwise operations, not the values (logical, numeric, or anything else). The results of bit fiddling on signed integral types are implementation defined.

    That's why it is a good idea to be consistent with use of operations. If the goal is a numeric result, use numeric operators and integral values. If the goal is a logical/boolean result, use logical operators on expressions that give logical/boolean values. If the goal is fiddling bits, use bitwise operators.

    Yes, there are some equivalences between the operators, but using a bitwise operation to fiddle with results from boolean or numeric expressions is problematical (e.g. if any of the values being fiddled are signed).

    Quote Originally Posted by Nominal Animal View Post
    There is a related trick in C that relies on int being twos complement is to substract one from the logical value, turning FALSE to all bits set, and TRUE to all bits zero. (All current widely-used processor architectures use twos complement format for signed integers.)
    Just because you can get away with it this year, doesn't mean you should code that way.

    The advantage of coding to your goal (as I described above) is that it is guaranteed to work on all past, current, and future systems. And it is easier for mere mortals to understand. In comparison, the trick you describe is not guaranteed to work on all current and past systems, and can easily fool programmers for no real benefit. Given the rate of evolution of computing technology, it would be a brave call to suggest it would work on future systems, just because it works on most current systems.

    Most mainstream compilers (both old and new) are smart enough to morph turn a numeric, bitwise, and logical operations into an efficient form on the target machine without requiring the programmer to use tricks like you describe. Such tricks can also fool the compiler into giving unintended results, let alone make things less efficient. Which is why such tricks are often described as premature optimisation.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

  15. #15
    Ticked and off
    Join Date
    Oct 2011
    Location
    La-la land
    Posts
    1,728
    Quote Originally Posted by grumpy View Post
    Which makes you still incorrect.
    What? I haven't been incorrect yet in this thread at all! All I have been is polite.

    You yourself are quite wrong, though.

    C89 does not have a boolean type, and all logical expressions (involving operators ! < > <= >= ==) evaluate to an int: 0 if false, 1 if true.

    That makes your assertion complete garbage.

    Since you seem to be unable to grasp the situation, let me break down the example you objected to:
    Code:
        some_flag |= (something && !something_else);
    Do you see the && operator? "The && operator shall yield 1 if both of its operands compare unequal to 0, otherwise it yields 0. The result has type int."

    This means it is strictly equivalent to
    Code:
        if (something && !something_else)
            some_flag |= 1;
    It does not matter a bit if something or something_else are signed values. In C89, all that matters is whether they are zero (false) or nonzero (true).

    There is only one problematic case with the above code snippet: If the C compiler uses one's complement representation for integers, and some_flag is -1, the logical value it represents will be incorrect if (something and not something_else). This is because -1|1 == -0 for one's complement representation. (However, if I remember correctly, the C compilers on those (UNIVAC?) actually treated only +0 as false, and -0 and all nonzero integers as true, thus "correcting" this case anyway.)

    (Bitwise operations with negative values using signed types is implementation-defined in C89. However, there have been only three representations for signed integers (sign and magnitude, one's complement, and two's complement), and only those three are allowed in C99 and later. In other words, even if the results are implementation-defined, there are only three options that implementations can actually choose from. Furthermore, in C99 and later the fixed-width integer types intN_t are defined to have two's complement representation without padding bits, so bitwise operations on negative values on those types are defined by the standard, not implementation-dependent.)

    Quote Originally Posted by grumpy View Post
    It is the type that matters when you do bitwise operations, not the values (logical, numeric, or anything else).
    That's just wrong.

    In particular, bitwise operations with signed types and positive values is quite well defined in C, but bitwise operations with signed types and negative values is implementation-defined. Obviously, the value matters.

    Furthermore, C89 yields ints from logical expressions (either 1 or 0). There is no logical type per se, unless you consider int such. Because logical expressions yield nonnegative integer results, you know that your bitwise operations are well defined in C (assuming the rest of the expression does not do bitwise operations on negative values or rely on other implementation-defined behaviour).


    Dammit, grumpy. You often sound quite sensible, and have good answers. Then, when I point something wrong in your post, no matter how gently and politely I try to do it, you go off flying into la-la land, spewing utter crap as you go. What the heck is your problem?

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Structs and arrays - overview and help neede
    By pistacchio in forum C Programming
    Replies: 7
    Last Post: 01-11-2010, 02:11 AM
  2. Can Any Experts Help With This?
    By xairzx in forum C Programming
    Replies: 6
    Last Post: 02-11-2009, 11:00 PM
  3. AI types - Brief overview?
    By Queatrix in forum General AI Programming
    Replies: 3
    Last Post: 11-09-2005, 12:17 PM
  4. General overview of comm port programming.
    By Sebastiani in forum C Programming
    Replies: 2
    Last Post: 02-26-2003, 01:53 PM