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.

Sign up for GitHub

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

Jump to bottom

Fix == and != when comparing signed vs unsigned integers #6689

Merged
merged 1 commit into from
Sep 10, 2018
Merged

Fix == and != when comparing signed vs unsigned integers #6689

merged 1 commit into from
Sep 10, 2018

Conversation

asterite
Copy link
Member

@asterite asterite commented Sep 9, 2018

Fixes #571

@asterite asterite added topic:compiler breaking-change kind:bug A bug in the code. Does not apply to documentation, specs, etc. and removed breaking-change labels Sep 9, 2018
@asterite asterite closed this Sep 9, 2018
@bcardiff
Copy link
Member

bcardiff commented Sep 9, 2018

Why closing it? I was up to send the approval.

@asterite asterite reopened this Sep 10, 2018
@asterite
Copy link
Member Author

@bcardiff Because it stucks at one spec, probably this broke something that depended on that equality. I'll reopen and investigate why it's failing, and try to fix that.

@asterite asterite force-pushed the bug/571-signed-unsigned-comparison branch from 5ffd993 to fa524cc Compare September 10, 2018 02:17
@asterite
Copy link
Member Author

Hopefully fixed. The iconv call returns size_t and an error is signaled as (size_t)(-1), so before this PR comparing an uint with -1 would work just fine, now it doesn't. Fixed by using LibC::SizeT::MAX instead of -1, but now I'm also marking this PR as a breaking change... what's worse is that this is a silent breaking change, but it might be better to do this now and not later.

@asterite asterite force-pushed the bug/571-signed-unsigned-comparison branch from fa524cc to d550776 Compare September 10, 2018 12:22
RX14
RX14 reviewed Sep 10, 2018
View reviewed changes
src/compiler/crystal/codegen/primitives.cr Outdated
@@ -161,6 +161,26 @@ class Crystal::CodeGenVisitor
end
end

# The below methods (lt, lt2, gt, gte, eq, ne) perform
Copy link
Contributor

@RX14 RX14 Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/lt2/lte/

Bit of a nitpick but it genuinely confused me for 30s

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed!

@asterite asterite force-pushed the bug/571-signed-unsigned-comparison branch from d550776 to 6ffffd0 Compare September 10, 2018 12:42
@RX14
Copy link
Contributor

RX14 commented Sep 10, 2018 •
edited
Loading

Amazing, you corrected the typo before I even had time to finish reading the whole diff!

@RX14 RX14 added this to the 0.27.0 milestone Sep 10, 2018
@asterite asterite force-pushed the bug/571-signed-unsigned-comparison branch 2 times, most recently from 96426d8 to cc2ab58 Compare September 10, 2018 13:40
@asterite
Copy link
Member Author

I keep adding fixes. This time, getting runtime integer values from LLVM returned UInt64 and we sometimes compared that with -1, so now it fails. I fixed it by making that used method to return Int32, while also allowing to get the values as Int64 and UInt64, which is what LLVM provides (but in the tests we don't use such big values so it's always safe to use Int32).

@RX14
Copy link
Contributor

RX14 commented Sep 10, 2018

CI still failing :C

@asterite asterite force-pushed the bug/571-signed-unsigned-comparison branch from cc2ab58 to 97271b9 Compare September 10, 2018 14:03
@asterite
Copy link
Member Author

CI passed 🎉

luislavena
luislavena reviewed Sep 10, 2018
View reviewed changes
spec/std/int_spec.cr Outdated
@@ -537,6 +537,18 @@ describe "Int" do
end
end

{% if compare_versions(Crystal::VERSION, "2.6.1") > 0 %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asterite is this version correct? Shouldn't be 0.26.1 instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Fixed :-)

@asterite asterite force-pushed the bug/571-signed-unsigned-comparison branch from 97271b9 to 086f372 Compare September 10, 2018 16:58
j8r
j8r reviewed Sep 10, 2018
View reviewed changes
spec/std/int_spec.cr
(x == y).should be_false
(y == x).should be_false
(x != y).should be_true
(y != x).should be_true
Copy link
Contributor

@j8r j8r Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to use should eq and should_not eq expectations to have a report of variables if it fails?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, here we are testing those operators and I wouldn't like to hide them behind a spec matcher.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is better as-is

@RX14 RX14 merged commit d8a1601 into crystal-lang:master Sep 10, 2018
@daliborfilus
Copy link
Contributor

You guys are awesome. Thank you.

ezrast pushed a commit to ezrast/crystal that referenced this pull request Oct 2, 2018
@asterite
Fix == and != when comparing signed vs unsigned integers (crystal…
ae9297e 
…-lang#6689)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j8r j8r j8r left review comments

@luislavena luislavena luislavena left review comments

@RX14 RX14 RX14 approved these changes

Assignees
No one assigned
Labels
breaking-change kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler
Projects
None yet
Milestone
0.27.0
Development

Successfully merging this pull request may close these issues.
6 participants
@asterite @bcardiff @RX14 @daliborfilus @luislavena @j8r