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

[math] added initializer_list initialization to matrix #1004

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

hshose
Copy link
Contributor

@hshose hshose commented Apr 20, 2023

Added constructor with initializer list to modm::math::Matrix class.

The huge diff is from the clang-format script... Not sure if you want to have this?

@salkinium
Copy link
Member

The huge diff is from the clang-format script... Not sure if you want to have this?

No, I think we came to the conclusion that we only change the formatting if we refactor the entire file completely.
Otherwise the changes become simply unreviewable.

@hshose
Copy link
Contributor Author

hshose commented Apr 20, 2023

Sure 😵‍💫 undid the clang-formating...

@hshose
Copy link
Contributor Author

hshose commented Apr 20, 2023

Maybe also not so super relevant to merge this, if the modm custom matrix math is abandoned anyways..

@salkinium
Copy link
Member

Maybe also not so super relevant to merge this, if the modm custom matrix math is abandoned anyways..

Kinda depends how long you can wait, cos I'm not very certain if eigen can really be intergrated until I try it.

@chris-durand chris-durand self-requested a review April 20, 2023 23:39
@chris-durand
Copy link
Member

@hshose I have committed the review suggestion to run the CI. Please test if the code works for you.

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Nice!

@hshose
Copy link
Contributor Author

hshose commented Apr 24, 2023

Yap, works, thanks.

@salkinium salkinium merged commit c87a62a into modm-io:develop Apr 24, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants