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

Stop shadowing haxe.io.Bytes. #1765

Open
wants to merge 2 commits into
base: 8.3.0-Dev
Choose a base branch
from

Conversation

player-03
Copy link
Contributor

@player-03 player-03 commented Feb 27, 2024

Lime's version of the class has fallen behind Haxe's over time, and I believe it's past time to return to using their version. Shadowing a core class should always be a last resort.

Lime does add a few things to the class, so we want to make sure we can still get the same functionality.

  • Lime adds @:autoBuild metadata to enable asset embedding. I used Compiler.addMetadata() to achieve the same.
  • [JS] Lime supports the lime_bytes_length_getter flag to allow setting length. This appears to be used by openfl-js, though there are other ways to achieve the same thing, so I submitted Make ByteArray compatible with Haxe's Bytes class. openfl#2692.
  • [JS] Lime adds some & 0xFF operations. The Haxe devs have gone on record saying this isn't needed, so I left them out.
  • [C++] Lime replaces #if cpp with #if (cpp || webassembly). Edit: this is redundant, because the cpp flag is set when targeting WebAssembly. I left it out.

I checked line by line in a diff editor to make sure that was everything. All other differences either had no functional effect (whitespace and so on), or were because Haxe's version was newer.

@joshtynjala
Copy link
Member

Shadowing a core class should always be a last resort.

Agreed. I'd even go as far as saying it should never be done. It's one thing to shadow another library's class, if you know the risks and there's seemingly no other choice. It's a giant can of Cthulhus to shadow a standard library class.

@joshtynjala
Copy link
Member

[C++] Lime replaces #if cpp with #if (cpp || webassembly). As far as I know there's no way to change #if statements without shadowing the file, so I set it up to do so specifically on WebAssembly. That version may fall behind over time, but at least everything else won't.

I may be showing my ignorance here, but doesn't webassembly support come from hxcpp? I'm surprised that the cpp flag isn't set when targeting wasm.

@player-03
Copy link
Contributor Author

player-03 commented Feb 27, 2024

I may be showing my ignorance here, but doesn't webassembly support come from hxcpp? I'm surprised that the cpp flag isn't set when targeting wasm.

I've never used Wasm, so I wouldn't know. I prefer to only make changes that I fully understand. But if we can test it and show that the cpp flag is set, then it should be safe to remove. (Currently looking up how to install the Emscripten SDK.)

@player-03
Copy link
Contributor Author

player-03 commented Feb 27, 2024

I tested, and the flag is indeed set. That makes things much simpler!

And it makes sense. Why would C++ functionality like the blit() function be available if the cpp flag wasn't set?

@player-03 player-03 mentioned this pull request Mar 14, 2024
@player-03
Copy link
Contributor Author

Update: this must be merged by the time Haxe 5 releases.

@player-03 player-03 changed the base branch from 8.2.0-Dev to 8.3.0-Dev October 22, 2024 16:37
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