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 crash on create new lua rotation object. #259

Merged
merged 1 commit into from Apr 7, 2019

Conversation

2 participants
@pirogronian
Copy link
Member

commented Apr 6, 2019

No description provided.

@pirogronian pirogronian requested a review from 375gnu Apr 6, 2019

@pirogronian pirogronian added the bug label Apr 6, 2019

@375gnu

375gnu approved these changes Apr 7, 2019

Copy link
Member

left a comment

please remove lines used for debugging, else is fine.

i believe the problem is that Eigen data should be aligned for SSE/AVX/etc while lua_newuserdata doesn't do that. So *q = qd; doesn't generates a proper code as it assumes that both q & qd are properly aligned. And that's why memcpy helps.

Nice work!

@@ -12,17 +12,17 @@
#include "celx.h"
#include "celx_internal.h"
#include "celx_vector.h"
#include <fmt/printf.h>

This comment has been minimized.

Copy link
@375gnu

375gnu Apr 7, 2019

Member

please remove this...


using namespace Eigen;

#define dbg fmt::fprintf(cout, __FILE__ "[%u]\n", __LINE__)

This comment has been minimized.

Copy link
@375gnu

375gnu Apr 7, 2019

Member

and these 2 lines

@pirogronian pirogronian force-pushed the celxrotfix branch from 79a552e to e3b7158 Apr 7, 2019

@pirogronian pirogronian merged commit c465ab2 into master Apr 7, 2019

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@375gnu 375gnu deleted the celxrotfix branch Apr 7, 2019

@375gnu 375gnu added this to Done in Release 1.7.0 Apr 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.