Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Issue 13230: std.variant.Variant Uses Deprecated .min Property in opArithmetic When T is a Floating Point Type #2386

Merged
merged 1 commit into from Aug 4, 2014

Conversation

MetaLang
Copy link
Member

@mihails-strasuns
Copy link

.min_normal is something very different from .min and it does not seem to be applicable here. I think proper fix is to harden static if conditions to avoid matching T : int for floating types - there is a dedicated branch for those after all.

@MetaLang
Copy link
Member Author

The compiler is telling me to use min_normal in place of min. If this isn't the correct action in all cases, then that deprecation message needs to be changed.

@mihails-strasuns
Copy link

I believe it is wrong deprecation message. More appropriate one may be .min value is not applicable to floating point values, please refactor your code if you expect it to be similar to int.min or use .min_normal if you actually need minimum positive normalized value.

I am not very educated in this topic though.

@MetaLang
Copy link
Member Author

Hold on a moment, why is typeof(float.max) returning true for implicit conversion to integral types in the first place?

@mihails-strasuns
Copy link

That is exactly the strange part. All there print false when I try it locally:

pragma(msg, is(typeof(float.max) : int));
pragma(msg, is(typeof(float.max) : ulong));
pragma(msg, is(typeof(float.max) : long));

@MetaLang
Copy link
Member Author

Is the deprecation message printed when you try to run the following code locally?

import std.variant;

void main()
{
    Variant v = 1.0f;
    //Deprecation: min property is deprecated, use min_normal instead
    v += 1.0f;
}

@yebblies
Copy link
Member

.min_normal is something very different from .min and it does not seem to be applicable here.

.min for floating point types is exactly the same thing as .min_normal in the compiler.

@MetaLang
Copy link
Member Author

.min for floating point types is exactly the same thing as .min_normal in the compiler.

Then this pull shouldn't change any behaviour other than silencing that deprecation message.

@mihails-strasuns
Copy link

Then this pull shouldn't change any behaviour other than silencing that deprecation message

This deprecation message highlights a bug. It effectively checks if T can represent negative numbers - but for floating point T.min will always be positive value.

btw I can see deprecation printed locally, will check how it gets there a bit later.

@yebblies
Copy link
Member

yebblies commented Aug 3, 2014

The message is being printed because even though the && is short-circuited, the later expressions still get semantic run on them. The correct solution is probably to use std.traits.isUnsigned (or similar) instead of just comparing .min. Alternatively, you could static if (isFloating...) the whole block.

@MetaLang
Copy link
Member Author

MetaLang commented Aug 4, 2014

The correct solution is probably to use std.traits.isUnsigned...

Works for me.

@yebblies
Copy link
Member

yebblies commented Aug 4, 2014

Auto-merge toggled on

yebblies added a commit that referenced this pull request Aug 4, 2014
Fix Issue 13230: std.variant.Variant Uses Deprecated .min Property in opArithmetic When T is a Floating Point Type
@yebblies yebblies merged commit cacd5f6 into dlang:master Aug 4, 2014
@MetaLang MetaLang deleted the variant-remove-float-min branch August 4, 2014 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants