Int32 overloads for sf::Time *, /, *= and /= operators

[eXpl0it3r]

Someone on IRC has brought up that multiplying an Int32 with an sf::Time object results in a compiler error, since it's both, implicitly converting to float or Int64, are legal and thus the compiler can't decide and marks it as ambiguous.

So for example this code will produce a compiler error:

sf::Time a = sf::seconds(1.f);
sf::Time b = a * 2;

use of overloaded operator '*' is ambiguous (with operand types 'sf::Time' and 'int')

Since one can't mark operators as explicit, I think we should add overloads for Int32 as well.

[Laurent]

But then we have the same problem with all other integer types (char, short, int, and their unsigned variants). We should rather wonder if we could handle math operations with a single overload rather than one for floats and one for ints.

[Nexus]

I don't think it's a good idea to provide more overloads, this will only lead to combinatorial explosion. Since the type would be converted internally, people should rather learn to use the correct type in the first place, so they do not unnecessarily lose precision.

The literal postfixes exist for a reason, even if most people don't bother using them

[eXpl0it3r]

One point I forgot to mention is that the returning API provides float (seconds), Int32 (milliseconds) and Int64 (microseconds). So not providing an overload for Int32 can seem a bit unbalanced.

[Laurent]

Yes, we need to support this feature. Not being able to multiply a time amount by a number is ridiculous. But adding an overload for sf::Int32 is not enough; one day someone will complain that his unsigned int factor cannot multiply his sf::Time value, and so on with other types.

What about providing a single overload using the 'double' type? I think it's precise enough to handle integer values without any loss.

[Nexus]

The true solution would be a template based on std::is_integral and SFINAE or tag-dispatching. But I don't really like that for something so simple (and I'm sure Laurent does so even less ), furthermore we would need to re-implement the type trait because we're in C++98.

What about providing a single overload using the 'double' type? I think it's precise enough to handle integer values without any loss.

I see a few problems with double:

• One could probably construct cases where it leads to rounding errors. Was the reason for providing sf::Int64 in addition to float overloads not to maintain guaranteed integer precision?
• While it allows implicit conversions from integer types, compilers will omit a warning in those cases -- one that you currently don't have for sf::Int64. Users with "treat warnings as errors" settings can't compile their code anymore.
• The change breaks code that relied on integer precision at runtime.

However, people who really care about precision could still use the conversion functions from/to microseconds. Still, the fact that we're taking these disadvantages only because people don't bother to use the correct types (and write 4ll instead of 4) doesn't really please me...

[dabbertorres]

The true solution would be a template based on std::is_integral and SFINAE or tag-dispatching. But I don't really like that for something so simple (and I'm sure Laurent does so even less )

Why would it be an issue for something so simple? It's a simple problem, so why not use a simple solution? (minus the waiting for C++11/14/17 though)


For now, what about an implicit cast to sf::Int64? Though, an overload for operator= (or overloaded constructor) would be needed as well.

Here's what I thought:

Time& operator=(Int64 i);

operator Int64() const;

Then that would allow arithmetic with whatever sf::Int64 can work with, and since sf::Time uses sf::Int64 internally, it should(?) work out.

That would allow people to do sf::Time a(5) and wanting 5 seconds, but I think that would be their fault for not reading the docs on what time unit the constructor expects. And I don't think that this would cause any compatibility issues (correct me if I'm wrong though!).

[Nexus]

Why would it be an issue for something so simple? It's a simple problem, so why not use a simple solution?

There would still be 2 (in case of SFINAE) or 3 (in case of type dispatching) methods for each operator. The implementation needs to be in the header, we need to trick Doxygen into making the API readable, and we need to specialize a traits template for 13 integral types. Using a template comes with further implications: implicit conversions are no longer considered in overload resolution, thus we again break code.

...not exactly my definition of simple

For now, what about an implicit cast to sf::Int64?

That defeats the entire purpose of having a type-safe time class.

(btw: casts are strictly speaking explicit conversions)

[dabbertorres]

Doxygen I can understand. I have some questions about your other points, but it's getting a bit off-topic!

For now, what about an implicit cast to sf::Int64?

That defeats the entire purpose of having a type-safe time class.
(btw: casts are strictly speaking explicit conversions)
[/quote]
Hmm. True, I guess you guys want sf::Time to be more than just a "unit conversion class" of sorts. Makes sense. (And thank you for the wording correction.)

[Jebbs]

The true solution would be a template based on std::is_integral and SFINAE or tag-dispatching. But I don't really like that for something so simple (and I'm sure Laurent does so even less ), furthermore we would need to re-implement the type trait because we're in C++98.

Why std::is_integral? Wouldn't std::is_arithmetic be better for this? That way you have only a single function for each operator instead of floats plus integer types. It would include double, but people should be aware of the implications and still allowed to use it if they want in my opinion.

[Nexus]

The idea would be to differentiate between integer and floating point types, not to artificially constrain the type. Non-arithmetic types won't work anyway (and if they do, there's no point in limiting them).