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

[cpp] Use cpp::Int64 for haxe.Int64 instead of a class #9935

Merged
merged 21 commits into from
Nov 8, 2022

Conversation

Aidan63
Copy link
Contributor

@Aidan63 Aidan63 commented Oct 28, 2020

Hello,
I've made an attempt at improving the haxe.Int64 type of the cpp target by having it be treated as the existing cpp::Int64 define (__int64 on msvc, int64_t otherwise) and updating several of the hxcpp structures to play nice with this change.

HaxeFoundation/hxcpp#932

  • haxe.Int64 is now an abstract around cpp.Int64, most of the maths helper functions have been removed but some are still used to avoid things like division operators in haxe returning a float.
  • clsIdArrayInt64 is a new object id and there is a new array store enum arrayInt64 as hxcpp arrays can now store cpp::Int64's without wrapping them in an object.
  • Int, String, and Object maps in hxcpp now have an Int64 store type and appropriate conversions as well as get and set functions for int64 values.

There are still some things I'm not sure about and could use improvements.

  • I haven't really used cppia before and while I tried to add a new array type for holding 64bit ints I couldn't get it to work and its a bit beyond me at the moment, so it uses an array of objects for 64bit ints.
  • cpp tests are failing on the ci as it needs a few functions I added in my hxcpp pr, cpp and cppia tests are passing locally for me. I also need to cleanup some of my gencpp.ml changes as I'm sure some of them aren't actually needed.

Cheers.

@skial skial mentioned this pull request Oct 28, 2020
1 task
@RealyUniqueName RealyUniqueName added the platform-cpp Everything related to CPP / C++ label Nov 2, 2020
@RealyUniqueName RealyUniqueName added this to the Backlog milestone Nov 2, 2020
@Aidan63
Copy link
Contributor Author

Aidan63 commented Sep 11, 2021

I've gone back and tried to update these two PRs to work with the latest dev branch. I've also added a new hash in hxcpp for int64 keys and added the cpp.Int64Map specialisation so maps with haxe.Int64 keys now work as expected. The Int64Helper extern is also used for all operations again. I think I missed that the implicit to int was why it was needed in the first place. Once thats fully gone at some point in the future operations can be definined on the cpp.Int64 type like the numeric extern abstracts for the C# target.

I'm having some trouble getting some things working as they were before. With the changes to cpp.Int64 where to and from haxe.Int64 conversions were moved into it along with the implicit to int now being a function, typing Std.isOfType(x, cpp.Int64) generates ::Std_obj::isOfType(x, ::hx::ClassOf< ::cpp::_Int64::Int64_Impl_ >()) instead of ::Std_obj::isOfType(x, ::hx::ClassOf< ::cpp::Int64 >()).
I'm guessing the _impl class is only generated when an abstract has functions and since you can't usually type check with an abstract one of the metas attached to it to skip that check. Removing the functions from cpp.Int64 gets it working again but I'm not sure what the actual fix would be.

Cppia is also giving me a bit of a hard time with the new cpp.Int64Map. When creating a cppia script using that type I get the Int64Map.hx:65: characters 39-61 : Unsupported operation in cppia :CppCode error. I know you can't use untyped in cppia but I've followed the same format as the other cpp maps and added the extra Int64Map checks into gencpp but I still can't get it to work out the box. Interestingly adding --macro exclude("cpp.Int64Map") to the cppia hxml gets it compiling fine and using maps with Int64 keys also works fine in cppia.

Any advice would be greatly welcome.

@Aidan63
Copy link
Contributor Author

Aidan63 commented Oct 23, 2021

Took another look at this and worked around the isTypeOf and abstract issue by intercepting it in gencpp.ml and replacing it with the correct path with CppClassOf.

Also found the HostClasses.hx file which has strings to auto exclude for cppia which fixes the Int64Map issue I was having.

I think this finishes everything feature wise, Int64Map works fine on cpp and cppia, all tests are passing as well when using hxcpp with the additions on that end.

@Aidan63 Aidan63 marked this pull request as ready for review October 23, 2021 17:24
@dazKind
Copy link
Contributor

dazKind commented Oct 2, 2022

This looks like a cool improvement. @hughsando, @Simn any thoughts or directions to get this merged?

@hughsando
Copy link
Member

I think that this is a worthwhile change.
It's been a little while since I've been over some of these finer points, but I'm tempted to think that if it passes the tests then it is probably better to get it in sooner rather than later.

As for making sure things always work when the versions haxe and hxcpp differ, you would ideally be able to make the hxcpp merge without the haxe merge and have things work as before. Then, you get the new native int64s when you update haxe.

You may need to update the HXCPP_API_LEVEL in the haxe generate project if you need to have hxcpp perform differently with new and old versions of haxe.

@Aidan63
Copy link
Contributor Author

Aidan63 commented Oct 15, 2022

I've just updated my PR on the hxcpp side to the latest hxcpp master and the mac tests are all passing (other platforms are failing with the same reason as the CI on master is failing).

The HxcppApiLevel define has already been bumped to 430 with the non blocking process exit code PR from a few months back so build.xml files are generated with 430 version which the new int64 stuff is guarded behind.

@Aidan63
Copy link
Contributor Author

Aidan63 commented Nov 7, 2022

I added a new enum getter for int64's as they would otherwise have to go though the object getter (boxing) or the int getter (truncated).

Now that the haxe CI is working again the hxcpp CI is and my PR on the hxcpp side is all green so it seems like I've not caused any backwards compatability issues. So that should be good to merge, then we can see what the haxe tests think of all this.

@Aidan63
Copy link
Contributor Author

Aidan63 commented Nov 8, 2022

nice, now that the hxcpp PR has been merged the CI is all green on the haxe side.

@Simn Simn merged commit bc50a04 into HaxeFoundation:development Nov 8, 2022
@Simn
Copy link
Member

Simn commented Nov 8, 2022

Thank you for your work, and sorry this took so long!

@Aidan63 Aidan63 deleted the hxcpp_native_int64 branch November 8, 2022 19:31
@skial skial mentioned this pull request Nov 9, 2022
1 task
@kLabz kLabz mentioned this pull request Apr 2, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-cpp Everything related to CPP / C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants