OKAY FAIL

[Ian]

// Boolean return codes (must be 0 or 1 only)
#define OKAY 0
#define FAIL 1

Why is this even a thing in supermodel? This seems like crazy bad design to me. If a function returns a boolean it's reasonable to expect that true would be success .. or whatever. If someone uses a boolean like a boolean

ie

bool result = Function();

if(result) {
some code;
}

They'll be in for a bad time if randomly OKAY is returned which is defined as zero lol. To fix this madness should use something like

enum class ErrorCodes { Okay, Fail };

Then bool types can't be confused.

[Bart]

I customarily use 0 as success (i.e., no condition) and non-zero as an error code (or just true for bool), similar to the POSIX convention, for functions that otherwise have no return type. I've seen both conventions in code bases. The reason I prefer this convention to this day over the increasingly more common true-on-success is that usually one wants to handle the error cases, not the success cases, so for me it feels really bizarre to have:

Code: Select all

if (!SaveFileToDisk(file))
{
  printf("error\n");
  return 1; // especially weird in main(), which uses my convention
}

The following has always felt more logical to me (like testing for a flagged result):

Code: Select all

if (SaveFileToDisk())
{
  printf("error\n");
  return 1;
}

I defined OKAY and FAIL to be explicit because it is just a convention after all. The intent was to be explicit about returning these rather than relying on boolean results directly (e.g. "return (foo == bar) ? OKAY : FAIL" rather than "return foo != bar".

An enum would be the better way to go in retrospect.

[Ian]

Yeah I understand the logic. In c you'd use something like errno_t which is simply a typedef for int. At least then the intention is clear. Any objections if we refactor this.

[Bart]

I think an enum would be good. Something like:

Code: Select all

enum Result
{
  OKAY,
  FAILED
};

[Ian]

Well I pushed the changes for this, i used enum class, then you can't accidentally mix the types, makes it more idiot proof. I didn't realise how much it was used all over the code base. Hopefully I've not broken anything.

There are a lot of classes with uninitialised variables. I know we've absolutely had bugs related to this before, would probably make sense to fix these.