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

Fixed hbytes compare #11610

Closed

Conversation

onehundredfeet
Copy link
Contributor

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.

tobil4sk and others added 30 commits April 11, 2023 09:48
* [pcre] Fix bytecode bindings issues

* [pcre] Fix another argument type

* Fix one more bytecode stub

* Fix minor typo
* Add white space around template type syntax

* removed trailing whitespace after comma in type parameter list
* Message reporting: namespace defines

* [tests] update message reporting defines

* [tests] update message reporting define for unit tests

* also rename no-color define internally

* update message reporting defines in get_signature
* Do not raise final assign in display

* handle `finalField|` hover case

* handle `finalField = value|` hover
…n#11192)

* [tests] add test for HaxeFoundation#11005

* Allow non constant values for inline variable init if -D no-inline

* [typer] use ctx.doinline instead of -D no-inline

* [tests] update test for 11005
* set EVars position from macro reification

* [tests] add test for 11162

* Expose Var.namePos

* Fallback to EVars position for var names when namePos is null_pos

* fix null_pos check
@Simn
Copy link
Member

Simn commented Mar 18, 2024

@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?

@skial skial mentioned this pull request Mar 18, 2024
1 task
@yuxiaomao
Copy link
Contributor

yuxiaomao commented Mar 19, 2024

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 kLabz changed the base branch from 4.3_bugfix to development March 19, 2024 07:42
@kLabz
Copy link
Contributor

kLabz commented Mar 19, 2024

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

@Simn
Copy link
Member

Simn commented Mar 19, 2024

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.

@Simn Simn closed this Mar 19, 2024
@kLabz
Copy link
Contributor

kLabz commented Mar 19, 2024

Cherry picked this commit into development.
00e2a93 (edit: + 790656d)

@kLabz kLabz added this to the 4.3 Hotfix candidates milestone Mar 19, 2024
@Simn
Copy link
Member

Simn commented Mar 19, 2024

Please also add the HArray version.

@yuxiaomao
Copy link
Contributor

Should we close #11468 ?

@Simn
Copy link
Member

Simn commented Mar 19, 2024

Ideally we add a test first.

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

Successfully merging this pull request may close these issues.

10 participants