C++ class for a flag register in a Chip8 emulator

Question

In order to learn C++ I decided to code a Chip8 emulator following a tutorial. When I came across the idea of a flag register and decided it would be fun to implement in order to get familiar with bitwise operations. I'd like this review to center on what can be improved from the point of view of C++, but of course any other suggestions and improvements are welcome.

class FlagRegister {
private:
    unsigned char state = 0x00;
    enum Flag: unsigned char {
        CF = 0x01,
        PF = 0x02,
        AF = 0x04,
        ZF = 0x08,
        TF = 0x10,
        IF = 0x20,
        DF = 0x40,
        OF = 0x80
    };

    bool checkFlag(Flag flagToCheck) {
        return (bool) ((this->state >> (flagToCheck - 1)) % 2);
    }

    void setFlag(Flag flagToSet) {
        this->state = this->state | flagToSet;
    }

    void unsetFlag(Flag flagToUnset) {
        this->state = (this->state & (~flagToUnset));
    }

public:
    bool checkCF() {
        return this->checkFlag(CF);
    }

    void setCF() {
        this->setFlag(CF);
    }

    void unsetCF() {
        this->unsetFlag(CF);
    }
// Idem for all other flags.
};

Answer 1

I see some things that may help you improve your program.

Avoid writing this

The overuse of this clutters the code and makes things harder to understand. For example, the current code contains this:

void setFlag(Flag flagToSet) {
    this->state = this->state | flagToSet;
}

It's much easier to read and understand if written instead like this:

void setFlag(Flag flag) {
    state |= flag;
}

Think about the user

Consider actually using this class. For example, to appropriately set the ZF flag we'd have to do something like this:

if (result == 0) {
    flags.setZF();
} else {
    flags.unsetZF();
}
return flags.checkZF();

That's way too much code for just one flag! Here's what I'd rather write:

return flags.Z(result == 0);

Use const where practical

I would expect that checking the value of a flag shouldn't alter any flag. To assure the reader of the code that this is the case, the functions should be marked const. This also tells the compiler to make sure that 1) the code implementing the function does not alter the associated object and 2) the code calling the function can be a const object. See this for a more detailed and rigorous explanation.

Simplify your code

Rather than having to go through another level of indirection, for such simple functions, I would remove the private methods. To use the syntax I advocated earlier, here's one way to write a pair of functions:

bool C() const {
    return state & CF;
}
bool C(bool val) {
    if (C() != val) {
        state ^= CF;
    }
    return val;
}

You could even get crazy with preprocessor macros and write this:

#define FLAGIZE(x) bool x() const { return state & x ## F; } \
    bool x(bool val) { if (x() != val) state ^= x ## F; return val; }

    FLAGIZE(C)
    FLAGIZE(P)
    // etc.

This macro is just a shortcut to writing all of those functions out by hand.

Consider multithreading

It may be convenient at some point to have multiple threads setting flags simultaneously. If that's the case, you want to make sure they don't overwrite, so either using something like std::atomic_uchar would provide a simple way to support such use.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. One good way to address that is by the use of comments. Another good technique is to include test code showing how your code is intended to be used.