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

Correct EnumValueMap implementation #9670

Open
wants to merge 7 commits into
base: development
Choose a base branch
from
Open

Correct EnumValueMap implementation #9670

wants to merge 7 commits into from

Conversation

RealyUniqueName
Copy link
Member

Fixes #2479

This is an implementation with correct EnumValueMap behavior for non-scalar enum parameters.

Target-specific implementations could be added as a next step to improve runtime performance.

@RealyUniqueName RealyUniqueName added this to the Release 4.2 milestone Jun 30, 2020
@skial skial mentioned this pull request Jul 1, 2020
Closed
1 task
@RealyUniqueName RealyUniqueName modified the milestones: Release 4.2, Backlog Dec 14, 2020
@RealyUniqueName RealyUniqueName mentioned this pull request Dec 14, 2020
Open
@Simn Simn self-assigned this Apr 11, 2022
@Simn
Copy link
Member

Simn commented Apr 11, 2022

Something fails on C#:

Command: haxe [compile-2.hxml,-D,fast_cast]
haxelib run hxcs hxcs_build.txt --haxe-version 4300 --feature-level 1
haxelib run hxcs hxcs_build.txt --haxe-version 4300 --feature-level 1
haxelib run hxcs hxcs_build.txt --haxe-version 4300 --feature-level 1 --out bin/main/bin/Main
src\haxe\ds\EnumValueMap.cs(48,38): error CS0029: Cannot implicitly convert type 'haxe.root.ValueType [d:\a\haxe\haxe\tests\misc\cs\csTwoLibs\bin\main\bin\haxeboot.dll]' to 'haxe.root.ValueType [d:\a\haxe\haxe\tests\misc\cs\csTwoLibs\bin\main\src\Type.cs(5)]'
src\Type.cs(5,15): (Location of symbol related to previous error)
src\haxe\ds\EnumValueMap.cs(49,39): error CS0029: Cannot implicitly convert type 'haxe.root.ValueType [d:\a\haxe\haxe\tests\misc\cs\csTwoLibs\bin\main\bin\haxeboot.dll]' to 'haxe.root.ValueType [d:\a\haxe\haxe\tests\misc\cs\csTwoLibs\bin\main\src\Type.cs(5)]'
src\Type.cs(5,15): (Location of symbol related to previous error)
Command exited with 1 in 5s: haxe [compile-2.hxml,-D,fast_cast]
test cs failed

@Simn Simn modified the milestones: Backlog, Release 4.3, Later Mar 24, 2023
@Simn
Merge branch 'development' into bugfix/2479-EnumValueMap
5266300 
# Conflicts:
#	std/haxe/ds/EnumValueMap.hx
@Simn
Copy link
Member

Simn commented Feb 9, 2024 •
edited
Loading

I have updated the branch. It doesn't have 7dc9272, which I would like to solve in a cross-platform way because I have the same issue on the JVM target. I think we can add Type.numEnumParameters to deal with this.

Edit: Actually that won't do because then we still have to create real arrays in order to check the individual elements. Not sure what a good API would look like here...

@Simn
Copy link
Member

Simn commented Feb 9, 2024

I've added the HL optimization back so that we can merge this. An improved API is blocked on #11568 which may or may not take a while longer to address.

I'm also not sure if this actually closes #2479 because the tests added here are pretty barebones, but I'll take Alex's word for it!

@Simn
Copy link
Member

Simn commented Feb 9, 2024

... I never really looked at the actual implementation here though. Building a map on top of arrays can be done, but linear cost for both read and write isn't acceptable for maps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinresol kevinresol kevinresol left review comments

Assignees

@Simn Simn

Labels
None yet
Projects
None yet
Milestone
Later
Development

Successfully merging this pull request may close these issues.

Enum map does not play nice with ValueType
3 participants
@RealyUniqueName @Simn @kevinresol