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 #264, builtin option can be safely used again with SWIG 4.0.2+ #342

Closed
wants to merge 3 commits into from

Conversation

adundovi
Copy link
Member

@adundovi adundovi commented Jun 8, 2021

Here is PR to fix #264, I've tested it with and without SWIG builtin enabled in cmake. Now, all tests pass regarding the vector3 representation, i.e., print(v), and the issue with keyword arguments. Also, the new test for checking out of range when setting or getting vector elements passes...

Copy link
Member

@TobiasWinchen TobiasWinchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why the JF12 test is needed here; the builtin change looks good to me. Build fails on MacOS though.

@adundovi
Copy link
Member Author

adundovi commented Jun 9, 2021

OK, I'll remove these tests. I used them just to make sure nothing is changed physically in the process. macOS fails because it lacks the FFTW support - i.e., all turbulence features are disabled in CI. By removing tests this will not be an issue any more.

@TobiasWinchen
Copy link
Member

The JF12 deprecation warning has nothing to do with the builtin, right? Would be cleaner to have this in a separate PR then.

@adundovi
Copy link
Member Author

adundovi commented Jun 9, 2021

The JF12 deprecation warning has nothing to do with the builtin, right? Would be cleaner to have this in a separate PR then.

Yes, that's true - I accidentally mixed those two commits.

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.

Reenable swig --builtin
2 participants