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

[tests] Add more Null op tests #11641

Merged
merged 7 commits into from
Jul 25, 2024

Conversation

yuxiaomao
Copy link
Contributor

When I made the HL implementation for null ops, I found that we can have some more spec tests

  • when doing </>/<=/>= between null values: it does not really make sens but I think it's good to ensure the same behavior
  • between null : Null<Int> and 0 : Null<Int>: it's different than Null<Int> with Int, at least the code generated by HL is different

It seems work as expected on HL, let's see if tests pass for other target.

@yuxiaomao
Copy link
Contributor Author

In summary:

Cpp fail because when comparing null <= null, it convert the value to 0 first (I don't think it's really a problem by itself).

Php fail because I added analyzer(ignore) on testOps. Probably the assign does not worked:

var k = false;
f((k = true) ? false : true);
t(k);

Lua: I can't understand the error 😢

@skial skial mentioned this pull request Apr 29, 2024
1 task
@Simn
Copy link
Member

Simn commented Jul 25, 2024

Could you update this branch? I'd like to see the exact cpp error.

@Simn
Copy link
Member

Simn commented Jul 25, 2024

After talking with @yuxiaomao we agreed that C++ is actually correct here because we basically test null >= null and that should be true, not false. That means that both the HL and JVM implementations need a fix.

@Simn Simn merged commit b537e99 into HaxeFoundation:development Jul 25, 2024
50 checks passed
@yuxiaomao yuxiaomao deleted the dev-tests-nullops branch July 26, 2024 06:19
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.

2 participants