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

cgen: fix infix expr with number overflow (fix #18905) #18936

Merged
merged 7 commits into from
Jul 23, 2023

Conversation

yuyi98
Copy link
Member

@yuyi98 yuyi98 commented Jul 21, 2023

This PR fix infix expr with number overflow (fix #18905).

  • Fix infix expr with number overflow.
  • Add test.
fn main() {
	a := u8(255)
	b := u8(1)
	c := a + b

	println(c)
	assert c == 0

	println(a + b)
	assert a + b == 0

	println(a + b < a)
	assert a + b < a
}

PS D:\Test\v\tt1> v run .
0
0
true

@spytheman
Copy link
Member

@yuyi98 Good work.

The PR is good on its own, but with it, code translated from C to V, will behave slightly and subtly differently than normal pure V code, and the same looking code, compiled with or without -translated can have different results.

If I understand it correctly, with it:

x := u8(5)
a := int(x * 123)
dump(a)
dump(typeof(a).name)

... will behave the same, as int(u8(x*123)), when x is u8, clipping the overflow, i.e. resulting in: [a.v:3] a: 103

Before, it behaved as int(x)*123, i.e. promoting the smaller type to a larger one, and then doing the multiplication, resulting in: [a.v:3] a: 615 .

I am not sure, if it should be merged. I can see it both ways :-| .

@medvednikov please take a look, and decide what is best for V in the long term here.

@spytheman
Copy link
Member

spytheman commented Jul 23, 2023

Here is the behavior of the same source code from above, without and with -translated:

#0 08:16:17 ᛋ fix_infix_expr_calc_overflow /v/vnew❱./v run a.v 
[a.v:3] a: 103
[a.v:4] typeof(a).name: int
#0 08:20:25 ᛋ fix_infix_expr_calc_overflow /v/vnew❱./v -translated run a.v 
[a.v:3] a: 615
[a.v:4] typeof(a).name: int
#0 08:20:29 ᛋ fix_infix_expr_calc_overflow /v/vnew❱

The difference in the generated C code is:

<       int a = ((int)((u8)(x * 123)));  // with this PR
---
>       int a = ((int)(x * 123)); // old behavior, or with `-translated`

@joe-conigliaro
Copy link
Member

we definitely need to fix the behaviour with -translated

@medvednikov
Copy link
Member

somehow C autocasts u8+u8 to an int?
I think Go's behavior is more correct and predictable
so I'm in favor of merging this
it can be handled by c2v, so that the -translated hack can be removed

@medvednikov medvednikov merged commit e1758bc into vlang:master Jul 23, 2023
48 checks passed
@yuyi98 yuyi98 deleted the fix_infix_expr_calc_overflow branch July 23, 2023 22:43
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.

Incoherent type coercion
4 participants