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

get_log_tx_scale: remove mutable variable #757

Merged
merged 2 commits into from
Nov 20, 2018
Merged

Conversation

tmatth
Copy link
Member

@tmatth tmatth commented Nov 19, 2018

No functional change.

@tmatth tmatth requested a review from ycho November 19, 2018 16:49
@tmatth
Copy link
Member Author

tmatth commented Nov 19, 2018

@ycho for your consideration.

There's probably a way of getting rid of the conditionals altogether with some clever log2 + bittwiddling.

src/quantize.rs Outdated
if num_pixels > 1024 { shift_bits += 1; };

shift_bits
(num_pixels > 256) as i32 + (num_pixels > 1024) as i32
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I tried exactly the same code in the beginning, borrowing from libaom, then rustc cannot pass it, since it does not do automatic type conversion (i.e. casting) from boolean to integer. :) Hence the CI raised red flag!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh you did casting, "as i32", sorry missed it-- )

Copy link
Collaborator

Choose a reason for hiding this comment

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

When casted, does rustc generate similar code as C? I,e, ture/false --> 1/0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use as or i32::from() like this

Copy link
Collaborator

Choose a reason for hiding this comment

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

and yes, boolean -> signed maps to 0/1 and not 0/-1 ^^

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok- thanks for confirm!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lu-zero , And thanks for example code for type conversion above!

Copy link
Collaborator

Choose a reason for hiding this comment

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

When the type may change i32::from() is suggested by clippy since would make refactoring easier. I consider that debatable.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lu-zero I think using i32::from() is reasonable.

@ycho
Copy link
Collaborator

ycho commented Nov 19, 2018

Do we know why there is red x over above with travis CI...?

@lu-zero
Copy link
Collaborator

lu-zero commented Nov 19, 2018

network problems on their side, I restarted it

@coveralls
Copy link
Collaborator

coveralls commented Nov 19, 2018

Coverage Status

Coverage increased (+0.03%) to 83.483% when pulling 97e2e0e on cosmetics/shift-bits into 6521320 on master.

@tmatth tmatth force-pushed the cosmetics/shift-bits branch 2 times, most recently from 7435edf to 69410dc Compare November 19, 2018 23:43
Copy link
Collaborator

@ycho ycho left a comment

Choose a reason for hiding this comment

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

Thanks!

@ycho ycho merged commit e324395 into master Nov 20, 2018
@tmatth tmatth deleted the cosmetics/shift-bits branch November 20, 2018 17:39
lu-zero pushed a commit to rust-av/rav1e that referenced this pull request Jan 8, 2019
* get_log_tx_scale: remove mutable variable

No functional change.

* quantize: add test for get_log_tx_scale()
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.

None yet

4 participants