Trying to make a simple binary counter in C++:

#include <iostream>
#include <condition_variable>
#include <cstdlib>
using namespace std;


int c1 = 0;
int c2 = 0;
int c3 = 0;
int c4 = 0;

int main ()
{
    condition_variable cv;
    for (int i = 15; i > -1; i = i - 1)
    {
        if ((c1 == 1) and (c2 == 1) and (c3 == 1))
        {
            int c4 = 1;
            int c3 = 0;
            int c2 = 0;
            int c1 = 0;
        }
        if ((c1 == 1) and (c2 == 1))
        {
            int c3 = 1;
            int c2 = 0;
            int c1 = 0;
        }
        if (c1 == 1)
        {
            int c2 = 1;
            int c1 = 0;
        }
        int c1 = (c1 + 1);
        
        system("clear");
        cout << c1 << " " << c2 << " " << c3 << " " << c4 << endl;
    }
}

...but clearly something is wrong. The output is completely messed up:

1651076200 0 0 0
1651076201 0 0 0
1651076202 0 0 0
1651076203 0 0 0
1651076204 0 0 0
1651076205 0 0 0
1651076206 0 0 0
1651076207 0 0 0
1651076208 0 0 0
1651076209 0 0 0
1651076210 0 0 0
1651076211 0 0 0
1651076212 0 0 0
1651076213 0 0 0
1651076214 0 0 0
1651076215 0 0 0

Why is it 1.6B instead of 1?

Trying to make a simple binary counter in C++:

#include <iostream>
#include <condition_variable>
#include <cstdlib>
using namespace std;


int c1 = 0;
int c2 = 0;
int c3 = 0;
int c4 = 0;

int main ()
{
    condition_variable cv;
    for (int i = 15; i > -1; i = i - 1)
    {
        if ((c1 == 1) and (c2 == 1) and (c3 == 1))
        {
            int c4 = 1;
            int c3 = 0;
            int c2 = 0;
            int c1 = 0;
        }
        if ((c1 == 1) and (c2 == 1))
        {
            int c3 = 1;
            int c2 = 0;
            int c1 = 0;
        }
        if (c1 == 1)
        {
            int c2 = 1;
            int c1 = 0;
        }
        int c1 = (c1 + 1);
        
        system("clear");
        cout << c1 << " " << c2 << " " << c3 << " " << c4 << endl;
    }
}

...but clearly something is wrong. The output is completely messed up:

1651076200 0 0 0
1651076201 0 0 0
1651076202 0 0 0
1651076203 0 0 0
1651076204 0 0 0
1651076205 0 0 0
1651076206 0 0 0
1651076207 0 0 0
1651076208 0 0 0
1651076209 0 0 0
1651076210 0 0 0
1651076211 0 0 0
1651076212 0 0 0
1651076213 0 0 0
1651076214 0 0 0
1651076215 0 0 0

Why is it 1.6B instead of 1?

• c++
• binary

• 5 Crank up the warnings: godbolt./z/KfKbn8Mdq . Lot of redefinition of variables in narrower scopes. – user4581301 Commented Mar 20 at 18:47
• 5 What on earth is condition_variable cv; doing in this program? – Ted Lyngmo Commented Mar 20 at 19:39
• 4 Probably something for Phase II of this program. Trust me on this, James, if you're heading in towards multithreading, you need to get a lot more comfortable with the language first. It's really easy to shoot yourself in the face with threads, and it's not something you want to learn to do while also learning the language fundamentals. – user4581301 Commented Mar 20 at 19:59
• 3 Worth repeating - enable your compiler warnings. You presumably have compiler warnings turned off, and you are now asking people what those warnings would have told you. – Drew Dormann Commented Mar 20 at 20:33
• 3 The answer by 463035818_is_not_an_ai is certainly correct and worth a read, but the fix may be too complicated if you just started learning C++. The minimal fix is remove all int s before the variables names inside main() so that you are using the existing variables instead of declaring new ones. After that (it should work, but it's bad coding practice) you should also move the declarations of the globals to the start of main(). Also, condition_variable is something completely irrelevant and you shouldn't need them until an advanced stage. – Weijun Zhou Commented Mar 21 at 3:40

1 Answer 1

You need to learn about scope. Your code has many variables that share the same name, but are different objects. Your code with more appropriate naming:

int c1_global = 0;
int c2_global = 0;
int c3_global = 0;
int c4_global = 0;

int main ()
{
    condition_variable cv;
    for (int i = 15; i > -1; i = i - 1)
    {
        if ((c1_global == 1) and (c2_global == 1) and (c3_global == 1))
        {
            int c4_1 = 1;
            int c3_1 = 0;
            int c2_1 = 0;
            int c1_1 = 0;
        }
        if ((c1_global == 1) and (c2_global == 1))
        {
            int c3_2 = 1;
            int c2_2 = 0;
            int c1_2 = 0;
        }
        if (c1_global == 1)
        {
            int c2_3 = 1;
            int c1_3 = 0;
        }
        int c1_4 = (c1_4 + 1);   // UNDEFINED! DONT DO THIS !
        
        system("clear");
        cout << c1_4 << " " << c2_global << " " << c3_global << " " << c4_global << endl;
    }
}

c1_4 is already in scope during its initialization but you cannot use its value. It's a quirk of the language that int c1_4 = (c1_4 + 1); is allowed (though warnings might warn you) and the line invokes undefined behavior due to reading from c1_4 before c1_4 is initialized. Printing a large number is one possible outcome.

The other counters you print are declared in global scope, initialized with 0, and never modified.

Inside the if blocks your code declares and initializes other variables with same name. Above I renamed them so it is more clear what object is accessed where. Once the block ends, at } the variables cx_1/2/3 lifetime ends.

Don't use global variables. Declare variables in the most narrow scope possible (but not more narrow). Avoid to declare variables with a name that shadows a variable from outer scope.

Never use a variables value during its own initialization (though, see Jerry Coffin's comment). Turn on warnings to let the compiler catch such mistakes.

So much for what's wrong on your code. The above code is wrong too. It invokes undefined behavior.

If you want to implement a counter I suggest you to write a custom class. It could for example wrap a std::array<bool> that stores the digits. I suggest to use std::bitset<4> a) to compare with your own implementation and b) to get accustomed to a useful type from the standard library.

To get started, this custom class should provide some means to increment the counter and to print the value on the screen. Overflow handling etc is left for the next step, once you have the basic counting working.

#include <iostream>
#include <array>
#include <bitset>

struct my_counter {
    void increment() {
        bool carry = true;
        for (auto& digit:data) {
            bool temp = digit and carry;
            digit = digit xor carry;
            carry = temp;
            if (carry == false) break;
        }
    }
    friend std::ostream& operator<<(std::ostream& out,const my_counter& self) {
        for (const auto& digit:self.data){
            out << digit;
        }
        return out;
    }
private:
    std::array<bool,4> data;
};

int main() {
    my_counter c1;
    std::bitset<4> c2;
    for (int i=0;i<16;++i) {
        std::cout << c1 << "\n";
        c1.increment();
        std::cout << c2 << "\n";
        c2 = {c2.to_ulong() + 1};
        std::cout << "------\n";
    }
}

Live Demo. I'll leave it to you to reverse the output of digits to match them the output of std::bitset if you want that.