Skip to content

Fix sys_stat size integer overflow#1157

Open
barisyild wants to merge 4 commits into
HaxeFoundation:masterfrom
barisyild:fix-sys-stat-size-int32-overflow
Open

Fix sys_stat size integer overflow#1157
barisyild wants to merge 4 commits into
HaxeFoundation:masterfrom
barisyild:fix-sys-stat-size-int32-overflow

Conversation

@barisyild

Copy link
Copy Markdown
Contributor

This pull request fixes the file size integer overflow issue for sys_stat.

@barisyild barisyild changed the title Fix sys stat size integer overflow Fix sys_stat size integer overflow Oct 14, 2024
@Simn

Simn commented Oct 14, 2024

Copy link
Copy Markdown
Member

CI failure is related:

Error: /Users/runner/work/hxcpp/hxcpp/src/hx/libs/std/Sys.cpp:479:4: error: conversion from 'long' to 'const Dynamic' is ambiguous
   STATF64(size);

@barisyild

Copy link
Copy Markdown
Contributor Author

CI failure is related:

Error: /Users/runner/work/hxcpp/hxcpp/src/hx/libs/std/Sys.cpp:479:4: error: conversion from 'long' to 'const Dynamic' is ambiguous
   STATF64(size);

I think Apple is a bit more strict with the type casting, the compile problem was solved.

https://github.com/barisyild/hxcpp/actions/runs/11325731311

@Apprentice-Alchemist

Copy link
Copy Markdown
Contributor

Note that long is 32-bit on certain platforms, it would be better to use int64_t.

@Simn

Simn commented Oct 14, 2024

Copy link
Copy Markdown
Member

Can't we just always use the __APPLE__ version?

@barisyild

Copy link
Copy Markdown
Contributor Author

Obviously nothing has changed on the apple side, but the error occurred, does anyone have any idea?

Old Commit:
a427d1f

New Commit:
193b3d1

@barisyild

Copy link
Copy Markdown
Contributor Author

Probably a bug, because the same code works fine in my ci.

https://github.com/barisyild/hxcpp/actions/runs/11331298885

@Aidan63

Aidan63 commented Oct 15, 2024

Copy link
Copy Markdown
Contributor

Doesn't this require a change on the haxe side to work properly? Since the FileStat typedef only has size as an Int.

Changes like this should also probably be guarded with a HXCPP_API_LEVEL check so new hxcpp versions can be used with older haxe without anything changing.

@barisyild

barisyild commented Oct 15, 2024

Copy link
Copy Markdown
Contributor Author

Doesn't this require a change on the haxe side to work properly? Since the FileStat typedef only has size as an Int.

Changes like this should also probably be guarded with a HXCPP_API_LEVEL check so new hxcpp versions can be used with older haxe without anything changing.

Even if there is no api change on the Haxe side, it looks fine, because haxe int actually works with float double-precision.

https://github.com/HaxeFoundation/haxe/blob/94bde7d3c00b0d1d5eabf0bbde93533c4a44b065/std/StdTypes.hx#L61

@barisyild

Copy link
Copy Markdown
Contributor Author

Doesn't this require a change on the haxe side to work properly? Since the FileStat typedef only has size as an Int.

Changes like this should also probably be guarded with a HXCPP_API_LEVEL check so new hxcpp versions can be used with older haxe without anything changing.

I tried and you are right, I think it needs a haxe api change.

@barisyild

Copy link
Copy Markdown
Contributor Author

@skial skial mentioned this pull request Oct 31, 2024
1 task
@SomeGuyWhoLovesCoding

Copy link
Copy Markdown

I just tried it and it doesn't work. FileStat.size just returns 0.

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.

5 participants