-
Notifications
You must be signed in to change notification settings - Fork 273
byte_extract lowering for complex_typet [blocks: #2068] #4228
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
byte_extract lowering for complex_typet [blocks: #2068] #4228
Conversation
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.
✔️
Passed Diffblue compatibility checks (cbmc commit: 5906993).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/101421654
byte_operands.insert( | ||
byte_operands.end(), | ||
sub_imag.operands().begin(), | ||
sub_imag.operands().end()); |
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.
use move instead
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.
How should I use move here given that I need to append to a vector? (The first insert of course should use move as you suggest.)
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.
std::move(sub_imag.operands().begin(), sub_imag.operands().end(), std::back_inserter(byte_operands));
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.
I'm now using std::make_move_iterator
as the std::back_inserter
would result in repeated reallocations. I hope that's ok as well.
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.
Yes I think that's fine, otherwise you would have to do a reserve
beforehand.
5906993
to
b5c3649
Compare
@romainbrenguier @smowton All comments addressed. |
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.
Looks good except that all the test code is commented. Happy to commit if there's an immediately-following PR that brings the tests in; otherwise suggest doing some interim check that this code is at least exercised.
When the operand is a complex_exprt we can just extract the respective member.
b5c3649
to
4469493
Compare
I had to add two small extra commits, but with those we now actually have a successful live test, and not a commented-out one. |
It may have worked before via the fallback to flattening of the entire expression to a bitvector, but let's be on the safe side and construct appropriate expressions.
4469493
to
ec3ee8e
Compare
It may have worked before via the fallback to flattening of the entire
expression to a bitvector, but let's be on the safe side and construct
appropriate expressions.