-
Notifications
You must be signed in to change notification settings - Fork 688
Use logical operators for bool converions. #2620
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
Use logical operators for bool converions. #2620
Conversation
|
@rtakacs What is the compilation error message on TizenRT? What type of |
|
As far as I understand TizenRT has a built-in "bool emulation" layer for whatever reason. In this layer bool is defined as uint8_t, and (bool)(a & b) is the last 8 bit of the result operation. Obviously this is not the same as the logical value of (a & b). |
| exponent + frac_digits, | ||
| &exponent, | ||
| ecma_number_is_zero (this_num)); | ||
| !!ecma_number_is_zero (this_num)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, ecma_number_is_zero returns bool and ecma_builtin_number_prototype_helper_round expects bool. I cannot think of any scenario with whatever bool redefinition where !! would be necessary. So, is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, that's indeed unnecessary change.
|
@zherczeg Yikes! Thanks for the background. |
|
@akosthekiss There is no compilation error message. The explicit conversation has runtime side effect that appears on some tests. |
2bacad7 to
dee2879
Compare
|
(I like Btw the TizenRT implementation: |
galpeter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
I kind of tend to agree with @zherczeg. |
I think we should follow the existing pattern ( PS: Self revision, however: 3 times is not "throughout the code base". |
|
@akosthekiss |
The explicit boolean type conversion (introduced by jerryscript-project#2575) desn't work with TizenRT if custom boolean representation is used. Of course, TizenRT gives an opportunity to use the C99 standard boolean (that works well if that is set). I've replaced all the explicit boolean type conversions with double negations that helps to work JerryScript well with custom boolean types. JerryScript-DCO-1.0-Signed-off-by: Roland Takacs rtakacs.uszeged@partner.samsung.com
dee2879 to
4f109c6
Compare
|
@LaszloLango @akosthekiss @zherczeg Thank you for your opinion. I've changed |
zherczeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still lgtm ;)
LaszloLango
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
akosthekiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The explicit boolean type conversion (introduced by #2575) desn't work
with TizenRT if custom boolean representation is used. Of course,
TizenRT gives an opportunity to use the C99 standard boolean (that works
well if that is set).
I've replaced all the explicit boolean type conversions with double
negations that helps to work JerryScript well with custom boolean types.
(Note: this patch fixes 14 tests on TizenRT.)