Patch undefined behavior stuff

[Architector4]

I ran the game with the undefined behavior sanitizer and played for an hour or two and found a few small undefined behavior things happening.

Some of them come from Allegro's _parallelogram_map function which apparently does a divide-by-zero and then multiplying with INT_MAX, and lot come from LuaJIT. A few came from the game code itself, which is what I patched up here.

[HeliumAnt]

Nice catches!
You can come by the discord if you need help/feedback/etc. btw. (If you don't use discord that's totally fine)

[Architector4]

You can come by the discord if you need help/feedback/etc. btw. (If you don't use discord that's totally fine)

I don't use Discord much at all (only as a browser tab lmao), but decided to join anyway. Immediately after joining it locked my account out entirely asking for phone number verification and insists that my real correctly typed phone number is invalid.

Well, I tried.

edit: Turns out, installing Discord then doing verification with exact same phone number then deleting discord immediately after helped. Guess I'm there now lol

[Causeless]

I don't think this makes sense. FLT_MAX is used as a sentinel value within the pathfinder itself to do infinite jump height. This is treated completely separately within the pathfinder, we can't just set it to an arbitrarily high value.

[Causeless]

Also- not too convinced about the bracket initialisation for bools and other simple types. I understand the desire for consistency, but it's less readable and obvious than the typical "= false".

The concept of default initialisation is just language lawyer rules for simple types as opposed to being directly defined within a visible constructor ala complex types, which feels like "consistency" is outweighing actual readability.

[HeliumAnt]

It's not really all about consistency, it's the modern c++ recommendation. I would especially prefer it for member init since it enforces strict value typing. We can say to have explicit defaults, but we need fixed defaults, in the place the member is defined (also default empty constructors exist for complex types, we literally use that everywhere, poorly, wayoa???). I've had enough of unitialized value problems.

[Causeless]

It's not really all about consistency, it's the modern c++ recommendation. I would especially prefer it for member init since it enforces strict value typing. We can say to have explicit defaults, but we need fixed defaults, in the place the member is defined (also default empty constructors exist for complex types, we literally use that everywhere, poorly, wayoa???). I've had enough of unitialized value problems.

I'm sorry, I don't understand. How does "bool m_x {}" enforce anything or resolve uninitialised values in any way that "bool m_x = false;" does not?

As far as I can see, the former is exactly equivalent to the latter except for being implicit in relying on the spec to define the default value.

Edit:

I mean within this very PR's earlier comments there was already confusion about the behaviour here. Which further solidifies my opinion that explicit is better than implicit.

[HeliumAnt]

bool foo = 0.0 is allowed bool foo{0.0} is not. The way we currently do things currently, this shit happens constantly (this has caused issues before). bool foo{} is guaranteed by spec to be 0 for all simple types, but I'm fine with having to explicitly specify bool foo{false} (which is also what I've been doing, now that I check).
struct Bar foo is fine but I'd prefer explicit defaulting by struct Bar foo{}

[Causeless]

That argument makes sense. Although, taken to its extreme, it would imply that usage of "bool b = false;" in ANY context would be inappropriate and incorrect; even on the stack. I guess this is really a case of the standard being too concerned with backwards compatibility to introduce fix things properly.

Anyways, that's fair, I suppose (although surely setting a bool to 0.0 is identical to false?). But under the circumstances I would prefer explicit initialisation to false under this situation.

I'd prefer that empty implicit initialisation is withheld for situations where it may communicate something else (i.e, this value is meant to be properly initialised later but we're default-initialising as a precaution).

[HeliumAnt]

That argument makes sense. Although, taken to its extreme, it would imply that usage of "bool b = false;" in ANY context would be inappropriate and incorrect; even on the stack. I guess this is really a case of the standard being too concerned with backwards compatibility to introduce fix things properly.

That is in fact the recommendation for modern c++

[Causeless]

Using awkward syntax to work around the refusal of the standard committee to fix its broken standard syntax doesn't feel like a good solution. It seems very fragile to have code quietly break depending on the preferred syntactic style.

Most other major codebases I've worked on enables compiler warnings-treated-as-errors for implicit or narrowing conversions- can we use that instead? Would require a few up-front fixes but would make things safe by default (including in non-initialisation contexts).

[Causeless]

Also- @Architector4

• Some of them come from Allegro's _parallelogram_map function which apparently does a divide-by-zero and then multiplying with INT_MAX

Can we fix these too? We're compiling Allegro from source as it has been modified.

[Architector4]

I don't think this makes sense. FLT_MAX is used as a sentinel value within the pathfinder itself to do infinite jump height. This is treated completely separately within the pathfinder, we can't just set it to an arbitrarily high value.

Oh, wasn't aware. It was causing undefined behavior due to getting casted to int.

I guess I should instead just add an explicit check at that spot in code for FLT_MAX and just use INT_MAX there? lol

...Also, since we're on the topic of code style, do I use these C style FLT_MAX INT_MAX identifiers, or should I do what looks saner and go for std::numeric_limits<float>::max?

[Architector4]

• Some of them come from Allegro's _parallelogram_map function which apparently does a divide-by-zero and then multiplying with INT_MAX

Can we fix these too? We're compiling Allegro from source as it has been modified.

Yeah, maybe sometime perhaps If I feel like it lmao

[Causeless]

I don't think this makes sense. FLT_MAX is used as a sentinel value within the pathfinder itself to do infinite jump height. This is treated completely separately within the pathfinder, we can't just set it to an arbitrarily high value.

Oh, wasn't aware. It was causing undefined behavior due to getting casted to int.

I guess I should instead just add an explicit check at that spot in code for FLT_MAX and just use INT_MAX there? lol

...Also, since we're on the topic of code style, do I use these C style FLT_MAX INT_MAX identifiers, or should I do what looks saner and go for std::numeric_limits<float>::max?

Search for s_JumpHeight, that's where it's checked. I guess INT_MAX should work for the vertical/diagonal checks, but then any value should as it's a separate code path entirely.

On the FLT_MAX, I don't care much as long as the identifiers are consistent.

Personally I both love and hate std::numeric_limits. It's nice to have a "consistent" templated method, but it's also shit because it's not actually consistent and min() has the exact opposite behaviour for floats vs signed integers (which I've personally seen has caused bugs in the past).

In any case I don't care all that much, as long as we choose one and stick to it.

[Architector4]

Personally I both love and hate std::numeric_limits. It's nice to have a "consistent" templated method, but it's also shit because it's not actually consistent and min() has the exact opposite behaviour for floats vs signed integers (which I've personally seen has caused bugs in the past).

Worth noting that this is the same behavior for FLT_MIN, and the real lowest value for floats is -FLT_MAX, or std::numeric_limits<float>::lowest lol

[Causeless]

Personally I both love and hate std::numeric_limits. It's nice to have a "consistent" templated method, but it's also shit because it's not actually consistent and min() has the exact opposite behaviour for floats vs signed integers (which I've personally seen has caused bugs in the past).

Worth noting that this is the same behavior for FLT_MIN, and the real lowest value for floats is -FLT_MAX, or std::numeric_limits<float>::lowest lol

Wow. Apparently GitHub just lets me edit other people's comments. So apologies for that- meant to quote reply.

Otherwise; yes, this is true. It's just that INT_MIN doesn't pretend to be consistent via templates, and you can also trivially go-to-definition to see what its value is (unlike with numeric limits). Though I'm not really making much argument one way or another moreso than just bitching about the standard.

[Architector4]

bitching about the standard

the most correct way to interact with C++ tbh

(unlike with numeric limits)

Consider making use of language server doc popups :V