Skip to content

Fix comparisons functions#3844

Merged
yebblies merged 1 commit intodlang:masterfrom
Safety0ff:wconv
Aug 6, 2014
Merged

Fix comparisons functions#3844
yebblies merged 1 commit intodlang:masterfrom
Safety0ff:wconv

Conversation

@Safety0ff
Copy link
Contributor

Likely fixes fix issue #8596.
The existing code had issues with overflow and implicit narrowing conversion.

@Hackerpilot
Copy link
Contributor

Looks reasonable.

@DmitryOlshansky
Copy link
Member

LGTM

@Safety0ff Safety0ff changed the title Fix potential issues caused by implicit narrowing conversion. Fix hash comparisons and other implicit narrowing conversions. Aug 6, 2014
@Safety0ff Safety0ff changed the title Fix hash comparisons and other implicit narrowing conversions. Fix comparisons functions Aug 6, 2014
@Safety0ff
Copy link
Contributor Author

This patch is trivial and good to go, @yebblies could you have a look at it please?

@quickfur
Copy link
Member

quickfur commented Aug 6, 2014

LGTM.

Wow, I've only recently been realizing how often the subtraction trick doesn't work. It works in asm because of the CPU's carry bit, but that's not expressible in the high-level language, and sometimes that single bit makes all the difference between an overflow (sign bit loss between int.max - int.min and int.min - int.max) and a correct comparison. I wonder how many more places this same bug is lurking in, I've just recently fixed a couple o' them in druntime. :-(

@yebblies
Copy link
Contributor

yebblies commented Aug 6, 2014

Me? ok.

@yebblies
Copy link
Contributor

yebblies commented Aug 6, 2014

Auto-merge toggled on

yebblies added a commit that referenced this pull request Aug 6, 2014
Fix comparisons functions
@yebblies yebblies merged commit 479e22d into dlang:master Aug 6, 2014
@Safety0ff Safety0ff deleted the wconv branch August 6, 2014 16:59
@Safety0ff
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants