-
Notifications
You must be signed in to change notification settings - Fork 276
Factor nondet choices in object factory #863
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
This splits the nondet-if-then-else concept into a utility. It only has one user here, but downstream branches already have several users. This reduces the diff between branches for easier future merges.
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.
Thanks a lot for bringing this up. I am to be convinced that this is the right approach; in fact I'd much rather have this reverted in test-gen-support and the patch proposed by @owen-jones-diffblue applied. I think it's much more concise.
init_code.move_to_operands(set_null_label); | ||
init_code.move_to_operands(init_done_label); | ||
} | ||
init_null_diamond.finish(); |
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 believe this is mis-indented.
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 it in the diff but isn't
|
||
\*******************************************************************/ | ||
|
||
inline exprt get_nondet_bool(const typet &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.
I'd suggest expr_util.{h,cpp} would be a good enough place to hold this one instead of creating a header-only module.
#include <util/symbol_table.h> | ||
#include <util/source_location.h> | ||
|
||
// This will be unified with other similar fresh-symbol routines shortly |
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.
When is that point in time?
Outputs: | ||
|
||
Purpose: Emits instructions and defines labels for the beginning of | ||
a nondeterministic if-else diamond. Code is emitted to the |
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.
It may be worth reading #767 front to back. Quoting the my key comment:
"It seems the code here really mixes up two concepts [...]: the code is being generated as if in the GOTO intermediate level, but it's being done using high-level language constructs. There does not seem to be any need for labels or gotos here. Just generate a code_ifthenelse with a then_case and an else_case. This avoids the static variable and synthetic labels, and will thus make the code much more concise."
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, true. I wrote this a long time ago when I thought that was how things were supposed to work here (since the Java frontend never generates high-level constructs, because bytecode). I'll take another look at this tomorrow.
@owen-jones-diffblue @tautschnig OK, I think however I still will want the splitting out of |
This splits the nondet-if-then-else concept into a utility. It only has one user here, but downstream branches already have several users. This reduces the diff between branches for easier future merges.