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

Fix overflow (and subsequent compiler warnings) in haxe.io.Bytes #6631

Closed
wants to merge 1 commit into from

Conversation

jgranick
Copy link

@jgranick jgranick commented Oct 2, 2017

Setting large Int 32-bit values directly were resulting in compiler warnings (such as https://github.com/openfl/lime/blob/develop/lime/graphics/format/BMP.hx#L89), and probably caused bad inlined values in C++ generated code due to wrapping.

@ncannasse
Copy link
Member

Bytes.set already does a & 0xFF on all platforms where it's required.
Which error are you trying to fix ? Please send a reproducible example as a separate issue.

@ncannasse ncannasse closed this Oct 3, 2017
@jgranick
Copy link
Author

jgranick commented Oct 3, 2017

Perhaps this is the problem, then:

https://github.com/jgranick/haxe/blob/c44d374e95e027ed0e2ba4e38ce1787bedbc08f5/std/haxe/io/Bytes.hx#L63

Constant 32-bit values (such as 0xFF000000) are cast as (int) for the inline calls, which the compiler warns is wrapping to different values.

@hughsando Do you think this is a real problem, or a warning we don't need to worry about? (and can somehow quiet)

@ncannasse
Copy link
Member

Could you give a reproducible example?

@hughsando
Copy link
Member

I don't think there will be any logic errors because the bit patterns will be the same, and setting a byte should automatically mask the bits it needs. I simple loop should verify this. The main issue would be if there was a compare in there, without a correct signed/unsigned cast.
So it would then become a question of removing the warnings, which may require some extra casting, depending on the warning. Adding a &0xff seems like an unnecessary overhead.

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.

None yet

3 participants