-
Notifications
You must be signed in to change notification settings - Fork 275
fix exprt::opX() accesses in cpp/ #4984
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
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.
Various requests for somewhat minor changes.
src/cpp/cpp_constructor.cpp
Outdated
side_effect_expr_function_callt &func_ini= | ||
to_side_effect_expr_function_call(initializer.op0()); | ||
side_effect_expr_function_callt &func_ini = | ||
to_side_effect_expr_function_call(to_unary_expr(initializer).op()); |
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.
This seems wrong: we above assert that it's a statement expression.
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.
The side_effect_expr_function_call is the operand of the statement expression.
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.
Ah, sorry, my comment was unclear: I think using to_unary_expr
on something that actually is a codet
seems wrong (even when the type system currently permits it).
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.
Now using side_effect_expr_statement_expressiont
src/cpp/cpp_typecheck_code.cpp
Outdated
assert(decl.op0().op0().id()==ID_symbol); | ||
value = decl.op0().op0(); | ||
DATA_INVARIANT( | ||
to_unary_expr(decl).op().op0().id() == ID_symbol, |
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 do we know that the .op0()
is safe here?
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.
added a check
assign.copy_to_operands(op0.as_expr()); | ||
assign.op0().add_source_location() = source_location; | ||
assign.copy_to_operands(op1); | ||
assign.copy_to_operands(op0.as_expr(), op1); |
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.
Could use the side_effect_exprt
constructor that also takes 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.
indeed, done
assign.op0().add_source_location() = source_location; | ||
assign.copy_to_operands(op1); | ||
assign.copy_to_operands(op0.as_expr(), op1); | ||
to_binary_expr(assign).op0().add_source_location() = source_location; |
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.
Doing to_binary_expr
on a side_effect_exprt
feels very wrong.
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.
now uses side_effect_expr_assignt
assign.op1().add_source_location() = source_location; | ||
|
||
to_binary_expr(assign).op0().add_source_location() = source_location; | ||
to_binary_expr(assign).op1().add_source_location() = source_location; |
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.
As above: use the side_effect_exprt
constructor that takes operands, and use some other approach instead of applying to_binary_expr
on a side_effect_exprt
.
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.
fixed as above
@@ -295,7 +294,7 @@ void cpp_typecheckt::default_assignop( | |||
cpctor.operands().push_back(exprt(ID_cpp_declarator)); | |||
cpctor.add_source_location()=source_location; | |||
|
|||
cpp_declaratort &declarator=(cpp_declaratort&) cpctor.op0(); | |||
cpp_declaratort &declarator = (cpp_declaratort &)cpctor.op0(); |
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 like a whitespace-only change, but maybe we can actually get rid of the C-style cast?
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.
done
assert(returned_value.id()==ID_dereference && | ||
is_reference(returned_value.op0().type())); | ||
assert( | ||
returned_value.id() == ID_dereference && |
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.
Seems unnecessary as the second conjunct now uses to_dereference_expr
.
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.
removed
src/cpp/expr2cpp.cpp
Outdated
assert(src.operands().size()==2); | ||
return convert(src.op0())+"["+convert(src.op1())+"]"; | ||
return convert(to_binary_expr(src).op0()) + "[" + | ||
convert(to_binary_expr(src).op1()) + "]"; |
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.
Could we use to_extractbit_expr
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.
done
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: 7f48ddc).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/121919890
Codecov Report
@@ Coverage Diff @@
## develop #4984 +/- ##
===========================================
- Coverage 69.56% 69.56% -0.01%
===========================================
Files 1314 1314
Lines 108922 108945 +23
===========================================
+ Hits 75776 75786 +10
- Misses 33146 33159 +13
Continue to review full report at Codecov.
|
8f1a0be
to
37fe7c7
Compare
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: 37fe7c7).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/123007942
887e2b1
to
e8c4be1
Compare
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: e8c4be1).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/124393808
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: f69bbac).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/131395568
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: 9ff2a56).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/131642359
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 for one new issue that needs to be fixed.
src/cpp/cpp_typecheck_code.cpp
Outdated
DATA_INVARIANT( | ||
to_code_decl(to_code(to_unary_expr(decl).op())).symbol().id() == | ||
ID_symbol, | ||
"declaration must have symbol as operand"); |
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.
This DATA_INVARIANT cannot fail, because to_code_decl
will already fail its code_declt::check(code);
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.
removed
This improves type safety.
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: 6457a5e).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/132910648
This improves type safety.