Fix Issue 22414 - clamp(a, b, c) should always return typeof(a)

[andralex]

Sigh.

[dlang-bot]

Thanks for your pull request, @andralex!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	22414	normal	clamp(a, b, c) should always return typeof(a)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8293"

[tgehr]

That doesn't seem right. E.g., I think it should be/previously was possible to clamp an int with lower and upper of type long.

[andralex]

Good point, I'll think of how to get that done.

[andralex]

It's interesting there were no unittests for such a case. Upon further thinking, I think such calls should be disallowed during compilation (if we agree that the return of clamp should be the same type as that of the clamped value). Consider:

int fun(int x)
{
    x = clamp(x, long.max, long.max);
    ....
}

This can't possibly work. x is always less than the left clamp, and furthermore the left clamp value cannot be represented as an int.

We could use assert or exceptions to weed out these cases, but the whole thing starts to smell of hiding bugs on the call site. Better disable the call entirely and compel the user to use appropriate types within the call.

I'll leave the code as is pending other arguments.

[JohanEngelen]

What is the outcome of clamping an int with uint values that int cannot represent?

int x = ...;
x = clamp(x, uint.max, uint.max);

[andralex]

@JohanEngelen thanks for raising the point. Per this PR is returns uint.max cast to int, i.e. -1.

This is in keeping with ordinary conversion rules, i.e. uint.max is entirely representable as an int and the representation's value is -1.

[andralex]

@JohanEngelen I added a unittest for that case.

[JohanEngelen]

Can you abstract it a little and document that behavior? (I think the behavior is that the comparisons are done "correctly", and that the outcome/clamped value is converted to the return type using standard integer conversion rules?)

[JohanEngelen]

merged without documentation improvement, nor author response to the comment...

[RazvanN7]

@JohanEngelen Oh, I'm sorry, I didn't properly inspect the date of your comment and the date of the last commit. Browsing through the PR page it seemed like a commit was made after your last comment. And the documentation does specify that "Comparisons are made correctly (using $(REF lessThan, std,functional) and $(REF greaterThan, std,functional)) even if the signedness of T1, T2, and T3 are different.". Compounded with the fact that the function returns T1, I thought that your comment is addressed.

However, I take it that you want the "using the standard integer conversion rules" added?

[JohanEngelen]

yeah. At least I did not know what x = clamp(x, uint.max, uint.max); would do from just that sparse documentation.

[JohanEngelen]

(this is a downside of the way PRs are merged. What works better, I think, is accepting a PR with a 'lgtm' and then the author commits the PR himself, where he can make last minute small changes. Or you can accept a PR with some nits, author fixes nits and merges PR... oh well :))