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

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.
};

• Welcome to Code Review! It would be helpful to reviewers (and yourself) if you provided a short program demonstrating the class in use. – Null Oct 16 '18 at 18:38

## 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;
}


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.

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.

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.
• With regards to this is it inferred if no other variable is declared with that name, for example in this (lol) case state. I didn't know that constcould be used for functions, is the purpose the one you stated or is it convention to use it that way without affecting what the function can and can't do? state & CF get casted to bool automatically withoud need to explicitly state it? Would FLAGIZE(C), f. e. be available from the outside or am I missunderstanding what it does? Multithreading: I'll try to take it into account when I learn it. I'll take your last point: will do! – HermanTheGermanHesse Oct 17 '18 at 7:46
• Is this inferred if no other variable has that name? – HermanTheGermanHesse Oct 17 '18 at 14:20
• Not exactly. It's (mostly) as though state were a local variable. Just as with local variables, any global state is shadowed by the one within the object. See en.wikipedia.org/wiki/Variable_shadowing#C++ – Edward Oct 17 '18 at 14:37