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

fix: folding of bitwise not literals #3222

Merged
merged 4 commits into from
Jan 20, 2023

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Jan 11, 2023

What I did

Fix #3218.

How I did it

Reuse (2 ** 256 - 1) - value from BitwiseNot instead of operator.inv, which can return a negative number.

How to verify it

See updated tests.

Commit message

fix: folding of bitwise not literals

Description for the changelog

Fix folding of bitwise not literals.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2023

Codecov Report

Merging #3222 (3bce207) into master (ae7d6d2) will increase coverage by 16.14%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master    #3222       +/-   ##
===========================================
+ Coverage   72.73%   88.87%   +16.14%     
===========================================
  Files          84       84               
  Lines       10580    10589        +9     
  Branches     2139     2208       +69     
===========================================
+ Hits         7695     9411     +1716     
+ Misses       2271      769     -1502     
+ Partials      614      409      -205     
Impacted Files Coverage Δ
vyper/ast/nodes.py 93.78% <100.00%> (+14.68%) ⬆️
vyper/codegen/ir_node.py 91.03% <0.00%> (+1.72%) ⬆️
vyper/abi_types.py 83.33% <0.00%> (+1.78%) ⬆️
vyper/ast/signatures/function_signature.py 88.28% <0.00%> (+1.80%) ⬆️
vyper/codegen/arithmetic.py 84.71% <0.00%> (+2.02%) ⬆️
vyper/codegen/context.py 91.91% <0.00%> (+2.94%) ⬆️
vyper/builtins/_signatures.py 97.64% <0.00%> (+3.52%) ⬆️
vyper/semantics/types/primitives.py 92.53% <0.00%> (+3.53%) ⬆️
vyper/codegen/self_call.py 95.65% <0.00%> (+4.34%) ⬆️
vyper/codegen/module.py 97.70% <0.00%> (+5.74%) ⬆️
... and 45 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tserg tserg marked this pull request as ready for review January 11, 2023 04:53
@tserg
Copy link
Collaborator Author

tserg commented Jan 11, 2023

If 0.4.0 is the next release, will it be a good time to remove the set of bitwise builtin functions that have been deprecated since 0.3.4?

@charles-cooper
Copy link
Member

If 0.4.0 is the next release, will it be a good time to remove the set of bitwise builtin functions that have been deprecated since 0.3.4?

let's worry about that later!

_op = operator.inv

def _op(self, value):
return (2 ** 256 - 1) - value
Copy link
Member

Choose a reason for hiding this comment

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

this is only valid for uint256 btw

Copy link
Member

Choose a reason for hiding this comment

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

it's going to be clearer imo as

Suggested change
return (2 ** 256 - 1) - value
return (2 ** 256 - 1) ^ value

the xor reflects what's going on with the bits. it's also going to generalize to other integer types a bit better. @tserg what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, I started with a xor mask before changing to its current form to match what was already in vyper.utils.

@charles-cooper charles-cooper merged commit deb3378 into vyperlang:master Jan 20, 2023
@tserg tserg deleted the fix/bitwise_not branch January 20, 2023 16:05
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.

Bitwise not operator fails on literals
3 participants