HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.03k stars 648 forks source link

Fixed hbytes compare #11610

Closed onehundredfeet closed 3 months ago

onehundredfeet commented 3 months ago

Fixed a missing case for comparison in HL/C generation. HBytes, when compared with null (or other HBytes) were falling through the comparison case and resulting in a 'Don't know how to compare error'.

Just added them in with a physical comparison which results in a C == comparison.

Simn commented 3 months ago

@yuxiaomao Could you confirm that this is good, and while we're here also add the HArray change from https://github.com/HaxeFoundation/haxe/pull/11568/files#diff-dfd1e9357feb0aa622873651a37a64af8cae2fcaaeee39b8b2c6d9139929dce5R747-R748?

yuxiaomao commented 3 months ago

Both changes seems good to me. Should I add the HArray change directly to this PR? Also, the commit should be merged into branch developement right?

Note: HLC test code

var pipes = new hl.NativeArray(1);
trace(pipes == null); // false
var bytes = new hl.Bytes(0);
trace(bytes == null); // true
var bytes = new hl.Bytes(1);
trace(bytes == null); // false
kLabz commented 3 months ago

Uh, this branch doesn't seem to like development branch. But yeah, PRs should target development, not 4.3_bugfix

Simn commented 3 months ago

I don't know what's happening here, it didn't look like this when I saw this yesterday... in that case it's probably easier if we commit these two changes (for both bytes and array) directly.

kLabz commented 3 months ago

Cherry picked this commit into development. 00e2a93ebe8dd2239b05bbc171a2d475f8a0a2ec (edit: + 790656da1d9bb6903adb9dd41ebc7edae11ba15f)

Simn commented 3 months ago

Please also add the HArray version.

yuxiaomao commented 3 months ago

Should we close https://github.com/HaxeFoundation/haxe/issues/11468 ?

Simn commented 3 months ago

Ideally we add a test first.