Remove `ABS` in favor of `Math::abs`

[KoBeWi]

<-

[kleonc]

Is the same planned for MIN/MAX/CLAMP/SIGN? If no, why doing so for ABS but not for the others?

[akien-mga]

I would do it for all of them indeed, but that's something we ought to discuss to see if there's any reason not to do this change.

[KoBeWi]

Other functions don't seem to have Math equivalents.

[reduz]

I want to note that ABS works fine for integers, wheread Math::abs is sort of broken (only for int, there should be more integer types exposed or else you get ambiguous conversion).

[KoBeWi]

I added int8_t and int16_t, so all integer types are covered now.

[akien-mga]

I'm still conflicted about this change I suggested, because of this inconsistency:

Is the same planned for MIN/MAX/CLAMP/SIGN? If no, why doing so for ABS but not for the others?

Let's rediscuss and assess for 4.3 to see if it's worth doing this change.

[deralmas]

Well, since #91283 popped up and discussion came again I wonder: what's the status of this PR? The last message talked about reassessing for 4.3 but now we're pretty ahead into that IMO :P

I'm still conflicted about this change I suggested, because of this inconsistency

IMO regarding that, considering that this is a pretty big codebase-wide change, I'd do one PR per method and do one of them at a time. Making a huge change at once would be a nightmare to review and handle, due to merge conflicts popping up constantly.

[akien-mga]

IMO regarding that, considering that this is a pretty big codebase-wide change, I'd do one PR per method and do one of them at a time. Making a huge change at once would be a nightmare to review and handle, due to merge conflicts popping up constantly.

Yeah of course, but the main question before doing one PR per method is whether we should do this change at all.

We should discuss the pros and cons of using methods vs macros for this. When I suggested this it made sense to me to consolidate all math operations in Math, but we then found that there are some drawbacks to using functions instead of plain pre-processor macros for those trivial operations.

[deralmas]

Yeah of course, but the main question before doing one PR per method is whether we should do this change at all.

Oh, I see.

I'm not enough of a C++ master to give a definite answer, some more expert feedback is definitely required.

That said, from my PoV, I'd be inclined to prefer a proper constexpr template method (as @Repiteo proposed), as they feel like a less finicky solution since, as far as I can tell, they should get inlined and optimized just like a macro, so I think that they'd compile to the same thing?

[Repiteo]

Just created #91324 as a minimal implementation of constexpr templates as math functions. It doesn't actually change any existing uses of the macros to the new implementations, so it should be a solid baseline for guaging any differences/issues with the existing macros.