[Core] Codestyle improvements to math types

[AThousandShips]

General style improvements to bring these classes into the code style standards

Removing unnecessary const and adding p_ and r_ prefixes when needed

[AThousandShips]

Found a few more cases that I'll add tomorrow

[Mickeon]

I wonder if explicitly defining const within these core classes has any performance benefit (likely not).

[Calinou]

I wonder if explicitly defining const within these core classes has any performance benefit (likely not).

For primitive types, it doesn't affect performance. It's purely about code style (it avoids accidentally reassigning a function parameter).

[lawnjelly]

What is the reasoning behind passing const real_t &p_val as opposed to const real_t p_val?

See e.g.: https://stackoverflow.com/questions/26387569/rule-of-thumb-for-when-passing-by-value-is-faster-than-passing-by-const-referenc

EDIT: Ah, I see, sorry, I see that this is what you were correcting. 😁

Blimey 😨 there's some of this in 3.x, we should correct it there too. You would hope the compiler would optimize past this in release, but I don't know what the guarantees require. And for debug who knows.

To quote, this is the danger:

Passing a value instead of a const reference has the advantage that the compiler knows the value isn't going to change. "const int& x" doesn't mean the value cannot change; it only means that your code is not allowed to change it by using the identifier x (without some cast that the compiler would notice). Take this awful but perfectly legal example:

static int someValue;

void g (int i)
{
    --someValue;
}

void f (const int& x)
{
    for (int i = 0; i < x; ++i)
        g (i);
}

int main (void)
{
    someValue = 100;
    f (someValue);
    return 0;
}

Inside function f, x isn't actually constant! It changes every time that g (i) is called, so the loop only runs from 0 to 49! And since the compiler generally doesn't know whether you wrote awful code like this, it must assume that x might change when g is called. As a result, you can expect the code to be slower than if you had used "int x".

The same is obviously true for many objects as well that might be passed by reference. For example, if you pass an object by const&, and the object has a member that is int or unsigned int, then any assignment using a char*, int*, or unsigned int* might change that member, unless the compiler can prove otherwise. Passed by value, the proof is much easier for the compiler.

[AThousandShips]

Nominally a primitive compiler might be helped by const to do some optimization but I'd say it doesn't matter for compilation speed

But a decent optimizing compiler would use some form of SSA analysis anyway which would handle detecting and collapsing changes to the variables anyway, though const could skip a step in that, though I'd say (from experience with LLVM) it would still do roughly the same analysis and restructuring, so any benefit would be negligeable (will do a full recompile just to compare)

Will push additional changes momentarily

Edit: No changes to compile time, it's all within error margins, both with and without these changes I get between 2:40-2:50 per build, so it just affects the code style (and makes it easier to read IMO, beyond just aligning with our style)

[AThousandShips]

The point about passing by reference is true! Didn't think of that, it is a strong reason to not pass simple types, will do a further sweep for such later and look at 3.x too!

I'd lean on using references for complex types but it's something to discuss for the future, can do some benchmarking later as well, I think the improvement by passing in a register (when passing a reference) is greater than the possible drawback of mutation issues (with possible exceptions for certain types of 64 bit builds, in a 64 bit build a Vector2i or a single precision Vector2 might be passed in a register as it can be packed into 64 bits)

But absolutely worth looking into for simple types, will do a sweep later today, thank you for these notes!

I'd assume a decent compiler would handle this, but no harm in improving code style and helping at the same time, will benchmark a release build later today with the current changes to see if it matters on that end

[lawnjelly]

Rule of thumb I've usually seen used:

• 32 / 64 bit primitive types, pass by value in all cases (unless e.g. using to return a value)
• complex types (especially over 64 bit) pass by const ref .. but profile / examine assembly in some cases to be sure, especially borderline cases

Also if it is non-pod and has complex copy constructors etc then by reference becomes more favourable so you can avoid these.

Compilers also have rules about strict aliasing etc to help prevent yourself shooting yourself in the foot, but imo it helps to explicitly tell it what you intend it to do. Also compilers are usually very smart about working out your intention (and will often use a faster way to achieve same), but sometimes they are constrained by rules for non-obvious situations.

[AThousandShips]

Prepared a 3.x version and will open a PR once a decision has been made on this PR (it's smaller in scope but has a few const X & cases so good to be free of

Also tested a release build on mingw (with disable_3d as it compiles a bit faster just for the benchmarking) and there was no discernable difference between this and master, I don't have llvm set up so no clue there, or other platforms, but I doubt it'll make any real difference either way

[akien-mga]

Thanks!

[AThousandShips]

Thank you!