Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#3848 closed defect (fixed)

Add and revert 64 bit integer script conversions

Reported by: elexis Owned by: echotangoecho
Priority: Must Have Milestone: Alpha 22
Component: Core engine Keywords:
Cc: Patch:

Description (last modified by echotangoecho)

Whenever one tries to send a variable of the type i64 (std::time_t) or u64from C++ to JS, the code compiles on linux but not on windows (happened with #3258 and r17928). The replay list currently convert the timestamp to a string and should be fixed. At that time, Itms added a change to source/scriptinterface/ScriptConversions.cpp so that the code compiles on windows, but that doesn't work on unix, see attachment:t3258_visual_replay_menu_v9.2.patch:ticket:3258.

Attachments (3)

int64_script_conversions_v1.patch (1.2 KB ) - added by elexis 8 years ago.
A patch that had been proposed before (but doesn't compile on linux iirc).
conversionsfix.patch (1.7 KB ) - added by echotangoecho 7 years ago.
Patch which may or may not work on windows.
conversionsfix.2.patch (4.2 KB ) - added by echotangoecho 7 years ago.
Add tests (just adapted the i32 and u32 conversions tests).

Download all attachments as: .zip

Change History (13)

by elexis, 8 years ago

A patch that had been proposed before (but doesn't compile on linux iirc).

by echotangoecho, 7 years ago

Attachment: conversionsfix.patch added

Patch which may or may not work on windows.

comment:1 by echotangoecho, 7 years ago

Description: modified (diff)
Keywords: patch rfc added
Milestone: BacklogWork In Progress
Owner: set to echotangoecho
Status: newassigned
Summary: 64 bit integer script conversions[PATCH] 64 bit integer script conversions

comment:2 by echotangoecho, 7 years ago

Additional note: the type of std::time_t is undefined.

comment:3 by Stan, 7 years ago

I'll try it tomorow evening. If nobody does before then. What should I look for

comment:4 by echotangoecho, 7 years ago

If it doesn't work, you will get compiler errors.

comment:5 by Stan, 7 years ago

Compiles fine on W10 64bits MSVC 120_XP Debug + Release modes. Game launches afterwards.

by echotangoecho, 7 years ago

Attachment: conversionsfix.2.patch added

Add tests (just adapted the i32 and u32 conversions tests).

comment:6 by elexis, 7 years ago

(21:13:12) Philip: Whoever was looking at 64-bit stuff: make sure you test it on 32-bit Linux too, because that's different to 32-bit Windows

comment:7 by Itms, 7 years ago

Resolution: fixed
Status: assignedclosed

In 19189:

Fix 64-bit integer script conversions by removing the discrepancy between Windows and Linux.

Patch by echotangoecho, fixes #3848.
Reviewed By: Itms

Differential Revision: https://code.wildfiregames.com/D84

comment:8 by Itms, 7 years ago

Keywords: patch rfc removed
Milestone: Work In ProgressAlpha 22
Summary: [PATCH] 64 bit integer script conversions64 bit integer script conversions

comment:9 by elexis, 7 years ago

Summary: 64 bit integer script conversionsAdd and revert 64 bit integer script conversions

comment:10 by elexis, 7 years ago

In 19367:

Revert 64bit number conversions added in r19189, don't add the previous long and unsigned long conversions back and use double in the Replay menu.

64bit conversions (including the long ones) are not safe, because not every number can be converted to the 253 JS numbers and pretending to do so is asking for bugs.
Explicitly use the double type in the Replay menu, because std::time_t is unspecified and some platforms like Ubuntu yakkety:i386 fail to build, looking for long.
Double should work for the next 285 million years, becomes consistent with SavedGame.cpp, is tested by test_ScriptConversions.cpp and doesn't pretend to cover all 64bit numbers.

Patch By: echotangoecho
Differential Revision: https://code.wildfiregames.com/D205
Refs #3848 D84 D112

Note: See TracTickets for help on using tickets.