Add universal sign support for index macros

[adamscott]

With this PR, ERR_FAIL_INDEX, ERR_FAIL_INDEX_MSG, ERR_FAIL_INDEX_EDMSG, ERR_FAIL_INDEX_V, ERR_FAIL_INDEX_V_MSG, ERR_FAIL_INDEX_V_EDMSG, CRASH_BAD_INDEX, and CRASH_BAD_INDEX_MSG now accept mismatched signed pair.

This PR removes also the UNSIGNED_INDEX macros, as they aren't needed anymore.
Edit: kept for compatibility reasons.

To test the PR, you can go see the implementation on the Compiler Explorer (godbolt)
Edit: new implementation: https://godbolt.org/z/cfbzf8876
Edit: bug in previous implementation, here's the fix: https://godbolt.org/z/db5aYc5bf

Supersedes #96302

[adamscott]

I don't know how to solve the msvc problem:

core\templates\cowdata.h(185) : error C2220: l'avertissement suivant est trait� comme une erreur
core\templates\cowdata.h(185) : warning C4723: division potentielle par 0

core\templates\cowdata.h:185

C4723 is potential divide by 0.

Edit:
I was able to debug it and I found that it's that line, the culprit. It seems that ONLY when compiling collision_polygon_2d.cpp, it seems that replacing this from the previous implementation of CRASH_BAD_INDEX with the new one fails:

godot/core/templates/cowdata.h

Lines 204 to 208 in 61598c5

	_FORCE_INLINE_ const T &get(Size p_index) const {
	CRASH_BAD_INDEX(p_index, size());
	return _ptr[p_index];
	}

In fact, it comes from this:

godot/scene/2d/physics/collision_polygon_2d.cpp

Lines 67 to 74 in 61598c5

	Vector<Vector2> segments;
	segments.resize(polygon.size() * 2);
	Vector2 *w = segments.ptrw();
	for (int i = 0; i < polygon.size(); i++) {
	w[(i << 1) + 0] = polygon[i];
	w[(i << 1) + 1] = polygon[(i + 1) % polygon.size()];
	}

If we change that code for the code below, it compiles great. (thanks msvc)

                Vector<Vector2> segments;
                segments.resize(polygon.size() * 2);
                Vector2 *w = segments.ptrw();
 
-               for (int i = 0; i < polygon.size(); i++) {
+               uint64_t polygon_size = polygon.size();
+               for (int i = 0; i < polygon_size; i++) {
                        w[(i << 1) + 0] = polygon[i];
-                       w[(i << 1) + 1] = polygon[(i + 1) % polygon.size()];
+                       w[(i << 1) + 1] = polygon[(i + 1) % polygon_size];
                }

Maybe it has to do with that:

This warning is issued only when optimizations are enabled.

Edit 2:

MSVC is really drunk. This compiles without warning:

                Vector<Vector2> segments;
                segments.resize(polygon.size() * 2);
                Vector2 *w = segments.ptrw();

 		for (int i = 0; i < polygon.size(); i++) {
 			w[(i << 1) + 0] = polygon[i];
-			w[(i << 1) + 1] = polygon[(i + 1) % polygon.size()];
+			int polygon_size = (i + 1) % polygon.size();
+			w[(i << 1) + 1] = polygon[polygon_size];
 		}

[huwpascoe]

IMO declare the evaluated size as source of truth for the whole thing.

const Size size = polygon.size();

if (size < 2) {
	return;
}

Ref<ConcavePolygonShape2D> concave = memnew(ConcavePolygonShape2D);

Vector<Vector2> segments;
segments.resize(size * 2);
Vector2 *w = segments.ptrw();

for (int i = 0; i < size; i++) {
	w[(i << 1) + 0] = polygon[i];
	w[(i << 1) + 1] = polygon[(i + 1) % size];
}

That way MSVC should discard the section entirely instead of puzzling over the potential of a mod zero.

[lawnjelly]

I hadn't really been following this, but I hope you also considered the alternative:

#define ERR_FAIL_INDEX(m_index, m_size)                                                                         \
	if (((unsigned int) m_index) >= ((unsigned int) m_size)) {                                                     \
		_err_print_index_error(FUNCTION_STR, __FILE__, __LINE__, m_index, m_size, _STR(m_index), _STR(m_size)); \
		return;                                                                                                 \
	} else   

I've seen this used very frequently. Two's complement of negative numbers will always be a larger unsigned value than the largest signed value. This same routine will thus work for unsigned and signed.

https://en.wikipedia.org/wiki/Two%27s_complement

Strictly speaking, in C standard it might support some ancient CPU that doesn't use two's complement and doing two checks is more correct, but in practice, I don't know if any modern CPUs this won't work on. You might find something from e.g. the 1960s that uses one's complement.

Compiler's likely know this trick too and probably reduce the two checks to a single unsigned check, I just thought it might make the code simpler, and it may work faster in non-optimized builds too.

The only thing you might have to watch out for is 32 bit / 64 bit when casting (you might have to detect the bit depth and cast appropriately). It also won't work for negative sizes, but I don't know if the existing routine will work correctly for that (it's usually the error in the index you are checking for afaik).

[RandomShaper]

Just here to praise this new approach of having a universal macro and, among other benefits, so taking away the cognitive load of deciding which macro applies to each case.

[huwpascoe]

I hope you also considered the alternative

The macro is supposed to check between completely different sized types, so don't think that applies here?

[lawnjelly]

The macro is supposed to check between completely different sized types, so don't think that applies here?

Maybe there is some obvious problem I'm missing, I think you need to elaborate though, I'm not clear on what you mean. The code I posted was illustrative BTW, as I said:

The only thing you might have to watch out for is 32 bit / 64 bit when casting (you might have to detect the bit depth and cast appropriately).

[huwpascoe]

I think you need to elaborate though

The macro is supposed to identify both out of bounds and UB on indexed container access with any mismatched integer types, at least is my understanding.

int8_t v = -1;
size_t s = 1024;
CRASH_BAD_INDEX(v, s); // crashes
CRASH_BAD_INDEX(255, s); // ok

The above example wouldn't work with the 2c version since converting to uint8_t is going to give 255?

[adamscott]

I made the check as a lambda.

• this makes the code way more readable, as we can create variables and such, and the whole condition is not on a single line.
• there's no penalty using a lambda, as the compilation just inserts the code inline.

I also use auto, as this limits the unnecessary casting of types, as this code is agnostic of how many bits ints may have.

[adamscott]

This is to make MSVC stop complaining that we could do a division by 0 if polygon.size() is 0.

[adamscott]

I created ___gd_is_index_out_of_bounds(m_index, m_size) instead of repeating the complex lambda every time.

I had some issues with some macros having updated versions of the lambda and others not.

[huwpascoe]

Revised:

#define ___gd_is_index_out_of_bounds(m_index, m_size)                                    \
	[&]() -> bool {                                                                      \
		using IndexType = std::decay_t<decltype(m_index)>;                               \
		using SizeType = std::decay_t<decltype(m_size)>;                                 \
		if constexpr (std::is_signed_v<IndexType>) {                                     \
			if (m_index < 0) {                                                           \
				return true;                                                             \
			}                                                                            \
		}                                                                                \
		if constexpr (std::is_signed_v<SizeType>) {                                      \
			if (m_size <= 0) {                                                           \
				return true;                                                             \
			}                                                                            \
		}                                                                                \
		using UnsignedIndex = std::make_unsigned_t<IndexType>;                           \
		using UnsignedSize = std::make_unsigned_t<SizeType>;                             \
		return static_cast<UnsignedIndex>(m_index) >= static_cast<UnsignedSize>(m_size); \
	}()

The second negative check can abort early if size is 0 since any access will be invalid.
The signed comparison can be removed since it's already determined both are positive.

[huwpascoe]

Ugh this is the worst game of whack-a-mole. More random changes, couple of default initializers and maybe it'll pass all checks.