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.

Sign up for GitHub

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

Jump to bottom

Refactor math::nq module #318

Merged
merged 13 commits into from
Apr 1, 2015
Merged

Refactor math::nq module #318

merged 13 commits into from
Apr 1, 2015

Conversation

hauleth
Copy link
Contributor

@hauleth hauleth commented Mar 8, 2015

No description provided.

nwin
nwin reviewed Mar 8, 2015
View reviewed changes
src/math/nq.rs
g: f64,
b: f64,
a: f64,
struct Template<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better name? Maybe Quad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be better.

@hauleth hauleth changed the title Refactro math::nq module Refactor math::nq module Mar 8, 2015
@nwin
Copy link
Contributor

nwin commented Mar 9, 2015

Can you please box the array? I had some bad experiences with stack sizes recently.

@hauleth
Copy link
Contributor Author

hauleth commented Mar 9, 2015

If it needed to be boxed type then Vec would be fine, but IMHO there is no need for either.

@nwin
Copy link
Contributor

nwin commented Mar 9, 2015

Well, Vec is fine. There is no gain in using an fixed size array. It only saves one allocation and maybe 256 bound checks. Point is, that the struct is now 2kb big.

@hauleth
Copy link
Contributor Author

hauleth commented Mar 9, 2015

I've brought Vec back, but changed syntax in new.

@nwin
Copy link
Contributor

nwin commented Mar 9, 2015

(y) the WIP still holds?

@hauleth
Copy link
Contributor Author

hauleth commented Mar 9, 2015

Still holds as I want to see if I can refactor sort and search. Probably some methods change their name.

@bvssvni
Copy link
Contributor

bvssvni commented Mar 25, 2015

Looks good!

@hauleth The new io has some dependency issues and we might want to wait until those get fixed to check whether it works.

@bvssvni
Copy link
Contributor

bvssvni commented Mar 31, 2015

@hauleth @nwin Can we merge this now?

@hauleth
Copy link
Contributor Author

hauleth commented Apr 1, 2015

I have no idea for now. Maybe later I find way to cleanup it more (or even extract it to another crate).

hauleth added a commit that referenced this pull request Apr 1, 2015
@hauleth
Merge pull request #318 from hauleth/cleanup-math-nq
3457723 
Refactor math::nq module
@hauleth hauleth merged commit 3457723 into image-rs:master Apr 1, 2015
@hauleth hauleth deleted the cleanup-math-nq branch April 1, 2015 09:19
@bvssvni
Copy link
Contributor

bvssvni commented Apr 1, 2015

Nice!

fintelia pushed a commit that referenced this pull request Dec 20, 2024
@HeroicKatora
Merge pull request #318 from MichaelMcDonnell/add-tinflstatus-message
e23208e 
Add TINFLStatus message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hauleth @nwin @bvssvni