Skip to content

Refactor processMeasurementTriple #404

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

Closed
wants to merge 1 commit into from
Closed

Refactor processMeasurementTriple #404

wants to merge 1 commit into from

Conversation

alberth
Copy link
Contributor

@alberth alberth commented Sep 24, 2015

Bunch of simple refactor steps to convert the function into a more readable version

@floe floe added this to the 0.2 milestone Sep 24, 2015
@alberth
Copy link
Contributor Author

alberth commented Sep 25, 2015

rebased to new master

@xlz
Copy link
Member

xlz commented Sep 26, 2015

Individual commits have really messy diffs so I didn't look.

I looked at the whole diff, and did not find any errors. Seems equivalent to me.

m_out[2] = tmp5; // ir amplitude
m_out[0] = ir_image_a; // ir image a
m_out[1] = ir_image_b; // ir image b
m_out[2] = ir_amplitude; // ir amplitude
Copy link
Member

Choose a reason for hiding this comment

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

These comments // ir amplitude are redundant now.

@alberth
Copy link
Contributor Author

alberth commented Sep 28, 2015

Individual commits are very small refactor steps that I did to ensure I wouldn't break current behaviour.
I can see it would seem quite messy, I'll have to find a better way.

For this branch, I additionally removed or shortened most comments, eliminated cond0 (replacing it with its value), and renamed "cond1" to "saturated". Finally, merged all commits into 1 "refactor" patch.

@xlz
Copy link
Member

xlz commented Jan 25, 2016

Manually merged.

@xlz xlz closed this Jan 25, 2016
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.

3 participants