5
\$\begingroup\$

I am given a short int. I need to print the bits of it into two bytes. My code is as follows:

#include <stdio.h>

union info{
    short z;
    struct data{
        unsigned a:1;
        unsigned b:1;
        unsigned c:1;
        unsigned d:1;
        unsigned e:1;
        unsigned f:1;
        unsigned g:1;
        unsigned h:1;
  };
}t;

union byte{
short n;
struct inside{
    char p:8;
    char q:8;
  };
}v;

int main(void)
{
 short x;

 scanf("%hd",&x);

 v.n=x;

 t.z=v.q;

 printf("%d %d %d %d %d %d %d %d\n",t.h,t.g,t.f,t.e,t.d,t.c,t.b,t.a);

 t.z=v.p;

 printf("%d %d %d %d %d %d %d %d",t.h,t.g,t.f,t.e,t.d,t.c,t.b,t.a);

 return 0;
}

My code gives the correct result. But I think that I've written an unnecessarily long code. Moreover, for larger bytes ( like in case of int ) , the process seems to be tiresome. Can I write the code in a simpler way ?

\$\endgroup\$
  • 3
    \$\begingroup\$ There's no union-find here, only union \$\endgroup\$ – harold Nov 5 '19 at 16:46
  • 2
    \$\begingroup\$ regarding: printf("%d %d %d %d %d %d %d %d\n",t.h,t.g,t.f,t.e,t.d,t.c,t.b,t.a); The fields in the union are unsigned, so the use of %d is an error. Suggest %hu \$\endgroup\$ – user3629249 Nov 5 '19 at 17:29
11
\$\begingroup\$

Avoid global variables - there's no need for t and v to exist outside main().

Always check the return value of scanf() before using the written values.

Don't assume that CHAR_BIT is 8, or that sizeof (short) is 2. Neither of those is portable.

Don't assume a particular ordering of bit fields within a struct - that's entirely compiler-dependent.

Portable code needs a loop to print the bits, like this:

#include <stdio.h>
#include <stdlib.h>

int main(void)
{
    short x;
    if (scanf("%hi", &x) != 1) {
        fputs("Input error\n", stderr);
        return EXIT_FAILURE;
    }

    unsigned short v = (unsigned short)x;

    unsigned short mask = -1u;                /* 11111... */
    mask -= (unsigned short)(mask / 2);       /* 10000... */

    while (mask) {
        printf("%d ", (v & mask) != 0);
        mask >>= 1;
    }
    printf("\n");
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ i actually intend to do it with unions... \$\endgroup\$ – Nehal Samee Nov 5 '19 at 16:49
  • 6
    \$\begingroup\$ @NehalSamee, you can not do it with short, unions and bit-fields, because no one guaranteed that short is 16 bits wide. You should use uint16_t instead which is exactly 16 bits wide. \$\endgroup\$ – eanmos Nov 5 '19 at 17:11
  • 1
    \$\begingroup\$ Should you care about non 2's complement, (unsigned short)~0; is a problem. Suggest -1u; \$\endgroup\$ – chux - Reinstate Monica Nov 5 '19 at 17:31
  • 1
    \$\begingroup\$ The way you're preparing mask is fairly awkward. Why not 1 << (8*sizeof(v) - 1)? That makes it more clear that it's just a 1 in the MSB. \$\endgroup\$ – Reinderien Nov 5 '19 at 19:20
  • 1
    \$\begingroup\$ 1 << (8*sizeof(v) - 1) is UB when USHRT_MAX == UINT_MAX - somewhat common - use 1u. 1 << (8*sizeof(v) - 1) assumes no padding - of course that applies 99.9999+% of the time. A direct setting of the MSBit of unsigned short mask is = USHRT_MAX - USHRT_MAX/2; \$\endgroup\$ – chux - Reinstate Monica Nov 5 '19 at 21:16
5
\$\begingroup\$

In addition to @Toby Speight's answer.

  1. There are only four types allowed for a bit field. There are the follows: signed int, unsigned int, int, and _Bool. So using char in this case:

    struct inside{
        char p:8;
        char q:8;
    };
    

    is implementation-defined.

  2. Use fixed integer types. In your code you use short the width of which is not well defined. If you want an object with a specific width, you should consider using fixed integer types from <stdint.h>, such as uint8_t or uint16_t.

  3. Use an anonymous struct inside the union. Your code actually is not valid, because a is not a member of t as well as p is not a member of v. a is member of the inner data struct:

    union info{
         short z;
         struct data{
                unsigned a:1;
                unsigned b:1;
                unsigned c:1;
                unsigned d:1;
                unsigned e:1;
                unsigned f:1;
                unsigned g:1;
                unsigned h:1;
         };
    }t;
    

    You have to use anonymous structures in order to compile your code.

  4. Bit fields are very implementation defined. You actually couldn't print bits of integer in a portable way because bit fields are very implementation defined. You have to care about a bit field width, about padding inside the structure, about allocation units inside the structure, about width of the integer, about byte ordering, and so on.

|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

I agree with most of the suggestions from @TobySpeight, except for the loop variable. Consider:

#include <stdlib.h>
#include <stdio.h>

int main(void) {
    short x;
    if (scanf("%hi", &x) != 1) {
        perror("Input error");
        return EXIT_FAILURE;
    }

    unsigned short v = (unsigned short)x;

    for (int i = 8*sizeof(v)-1; i >= 0; i--)
        printf("%u ", 1&(v>>i));
    putchar('\n');
    return 0;
}

You can have a simple integer loop variable. The way this works:

  • i starts at 15, and decreases to 0
  • for every digit, shift the number right by i, so that the digit in question is in the least-significant position
  • Do a binary-and with 1, and then print the result.
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Minor: int i = 8*sizeof(v)-1; assumes no padding in unsigned short. Certainly not a significant concern. \$\endgroup\$ – chux - Reinstate Monica Nov 5 '19 at 21:17
  • \$\begingroup\$ I think you misspelt CHAR_BIT there: for (int i = CHAR_BIT * sizeof v - 1; i >= 0; --i) \$\endgroup\$ – Toby Speight Nov 6 '19 at 8:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for?Browse other questions tagged or ask your own question.