-
Notifications
You must be signed in to change notification settings - Fork 274
Replace all uses of deprecated symbol_exprt constructors [blocks: #3768] #3766
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
Replace all uses of deprecated symbol_exprt constructors [blocks: #3768] #3766
Conversation
538612f
to
4269207
Compare
4269207
to
115efb3
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: 115efb3).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/97091511
@@ -170,7 +170,8 @@ void java_simple_method_stubst::create_method_stub(symbolt &symbol) | |||
symbol_table); | |||
const symbol_exprt &init_symbol_expression = init_symbol.symbol_expr(); | |||
code_assignt get_argument( | |||
init_symbol_expression, symbol_exprt(this_argument.get_identifier())); | |||
init_symbol_expression, | |||
symbol_exprt(this_argument.get_identifier(), this_argument.type())); | |||
get_argument.add_source_location() = synthesized_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.
This one does appear to be a bugfix!
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, but it should actually get merged via #3747, I'll then rebase this PR on top of it.
@@ -215,7 +215,7 @@ SCENARIO("instantiate_not_contains", | |||
simplify(sc.premise, empty_ns); | |||
simplify(sc.s0, empty_ns); | |||
simplify(sc.s1, empty_ns); | |||
witnesses[sc] = generator.fresh_symbol("w", t.witness_type()); | |||
witnesses.emplace(sc, generator.fresh_symbol("w", t.witness_type())); | |||
nc_axioms.push_back(sc); |
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.
Not sure that the .emplace adds a lot here (exprt is very cheap to construct).
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 .emplace
is necessary, because a symbol_exprt
cannot be default constructed.
symbol_exprt(param.get_identifier(), param.type()); | ||
locals_map.emplace( | ||
param.get_identifier(), | ||
symbol_exprt(param.get_identifier(), param.type())); | ||
} |
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.
Same here
src/goto-instrument/concurrency.cpp
Outdated
@@ -58,14 +58,14 @@ class concurrency_instrumentationt | |||
{ | |||
public: | |||
typet type; | |||
symbol_exprt array_symbol, w_index_symbol; | |||
exprt array_symbol, w_index_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.
Can these ever be something that's not a symbol_exprt?
If initialisation is delayed, then perhaps optionalt<symbol_exprt> is the right way to say that.
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.
Well, the code is not particularly complete. These are actually never set at the moment.
@@ -409,7 +409,7 @@ void goto_symext::locality( | |||
|
|||
if(c_it != state.level1.current_names.end()) | |||
{ | |||
frame.old_level1[l0_name]=c_it->second; | |||
frame.old_level1.emplace(l0_name, c_it->second); | |||
c_it->second = std::make_pair(ssa, frame_nr); |
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.
Same here
/// Constructor | ||
/// \param expr: Expression to be converted to SSA symbol | ||
explicit ssa_exprt(const exprt &expr): | ||
symbol_exprt(expr.type()) | ||
explicit ssa_exprt(const exprt &expr) : symbol_exprt(ID_nil, expr.type()) | ||
{ |
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 is the first time you've used the nil ID -- before it has been the empty one.
If you are concerned about any time taken to convert "" into an irep_idt, then use irep_idt().
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 irep_idt()
here.
115efb3
to
2a5c14a
Compare
Use optionalt<symbol_exprt> when it isn't always set [blocks: #3766]
…ation Use consistent types with function_application_exprt [blocks: #3766]
272b62c
to
7c9aaff
Compare
This PR now only has a single commit left, (re-)reviews would be appreciated. |
7c9aaff
to
697a744
Compare
697a744
to
c539028
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: c539028).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/98341703
c539028
to
45c74ce
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: 45c74ce).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/98712690
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 bits that touch my areas of responsibility seem fine.
: term(_term), name(_name) | ||
{ | ||
} | ||
|
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.
+1 this is not clear.
45c74ce
to
8bea959
Compare
8bea959
to
d16fbeb
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.
🚫
This PR failed Diffblue compatibility checks (cbmc commit: d16fbeb).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/98896786
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.
This helps type safety has it avoids constructing symbol_exprts that never get a proper type (or identifier).
d16fbeb
to
267094c
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: 267094c).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/99079556
This helps type safety has it avoids constructing symbol_exprts that never get
a proper type (or identifier).