I am writing a code to find the number of ones in binary representation of a number

here is the code:

int main(void)
{
    int n,tNum,count = 0;
    cin >> n;
    tNum = n;
    while(tNum > 0)
    {
        int i = 0;
        int bit = getBit(n,i);// get bit
        if (bit == 1)
        {
        count  ;
        }
        tNum >> 1;
        i  ;
    }
    cout << count << endl;
}

Above code gives an endless loop and tNum didn't change its value I didn't understand where I am doing wrong

CodePudding user response：

Bitwise right shift operator >> didn't worked as intended

The bitwise right shift operator >> is working as intended but you are not storing the result of >> operator anywhere. This expression

tNum >> 1;

will right shift the value of tNum by 1 but the result of this expression is unused.

Use >>= operator instead of >>. It should be:

        tNum >>= 1;

This is same as tNum = tNum >> 1.

Another problem in your code:

This

getBit(n,i);

will give the 0^th bit of n (as the i is initialised with 0 in every iteration) and which is going to be same for every iteration. Instead, you should get the 0^th bit of tNum as you are using shift operator on tNum below in your code. Also, you don't need i at all, just remove it. The statement should be:

int bit = getBit(tNum, 0);// get 0th bit

One more important point, the result of right shift >> of a negative number is implementation defined. May, you should use unsigned int type for n and tNum.

You can do:

int main (void)
{
    unsigned int n, tNum;
    int count = 0;

    cin >> n;
    tNum = n;
    while(tNum > 0)
    {
        int bit = getBit(tNum, 0);// get bit
        // You can also do
        // int bit = tNum & 1;

        if (bit == 1)
        {
            count  ;
        }
        tNum >>= 1;
    }
    cout << count << endl;

    return 0;
}

Note: There is scope of improvement in the implementation of your code. I am leaving it up to you to identify those improvements and make respective changes in your code.

CodePudding user response：

You either don't need the i or you don't need to shift tNum, decide which one.

For example:

    int count = 0;
    while(tNum > 0)
    {
        count  = getBit(n,0);
        tNum >>= 1;
    }

CodePudding user response：

You initialize int i = 0; at the start of the loop so i doesn't do anything useful. Also, you don't update tNum. Consider using a for() loop instead which implies moving the initialization outside the loop:

for(int i = 0; tNum > 0; i  ) {
   ...
   tNum >>= 1;
}

As you only use n in the call to getBit() you could refactor it to eliminate tNum. Prefer using unsigned values when shifting. It's not clear (to me) if a bit count of a negative value would count the sign bit.

#include <iostream>
using namespace std;

int main(void) {
    unsigned n;
    cin >> n;
    unsigned count = 0;
    while(n) {
        if(n & 0x1) count  ;
        n >>= 1;
    }
    cout << count << endl;
}

Finally, modern processors usually have an instruction for this (popcnt) which you compiler might wrap for you or you can access it via an intrinsic (there are many versions available).