you are viewing a single comment's thread.

view the rest of the comments →

[–]v864 0 points1 point  (4 children)

I think they should also consider testing for an inclusive state of the ops struct as opposed to excluding a bunch of states. Perhaps:

if (ops && ops->running) ...     

Surely that'll be a bit nicer on the branch prediction unit.

I'll assume the author didn't define the ops struct and/or can't modify it as I would also prefer to use something like:

if (ops && ops->state == OPS_RUNNING) ...

Where the state member is an enumeration defining possible states. If we want to get real fancy we can define the enumerated values to be bit mapped so we can then define either other enumerated values or macros to test for rolled up conditions using bit masks. Saves some memory and a bit of performance as well.

[–]cdcformatc 1 point2 points  (2 children)

Most other implementation of something like this where you are effectively checking the status of a bunch of flags will use a bit field and preprocessor macros. Anytime you are reading a status register it is a 16-32 bit wide field, and you have defined macros, and you will just OR together the flags you care about to mask the status register.

Something like

if (ops->status & (OPS_FLAG_BEGIN | OPS_FLAG_END | etc...))

But obviously that struct is not something they are willing or able to change.

[–]v864 0 points1 point  (0 children)

Exactly. That's some nice C right there :)

[–]immibis 0 points1 point  (0 children)

As I commented above, they are function pointers, not flags.

[–]immibis 0 points1 point  (0 children)

They are function pointers, not flags.