-
Notifications
You must be signed in to change notification settings - Fork 284
Test gen support string additional options #695
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
Test gen support string additional options #695
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.
Use const
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.
const?
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.
( on previous line
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.
make const
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 seems that making substitute_* do an in-place substitution could simplify the code.
for(auto &op : expr.operands())
substitute_array_access(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.
I've made substitute_array_access in-place 14a1138
But I've let substitute_array_with_expr as it is because I think it is simpler in the current state.
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.
break after (
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 loop would be simpler if you considered the first element as default case of the ite chain. Is there a formula readability argument against this?
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 readability would be fine with first element as default, but I don't see why it would make the loop simpler. It would just shift the bounds by 1?
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.
What is the meaning of -1 ?
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 prefer using an unsigned integer and std::numeric_limits::max() as default upper limit. What is "no limit" supposed to mean in your case? What is the relation to MAX_CONCRETE_STRING_SIZE?
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.
auto &it (several instances)
src/cbmc/cbmc_parse_options.h
Outdated
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 suggest to put each option on a separate line to facilitate merging.
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.
Is ulong needed here? It's initialised from uint.
|
Please remove the [string-refine] prefix from commit messages. |
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.
Why do we need a long 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.
This is because length of string in java are 32 bits integer, while int in c++ on some systems could be 16 bits if I'm not mistaken
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, minimum size of int is 16 bits. For Java, int32_t would make it independent of the system. For other languages, this might be different. Eg c++ string::max_size() tells me something close to 2^64 on my machine.
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 pointed out earlier, this loop is not nice. The mixture of long, unsigned, size_t etc is likely to cause trouble. I dig up my earlier proposal that does without indexing.
Added field in string_refinement and string_constraint_generator to bound size of strings in the program and enforce a range on the characters.
We add the options string-max-length, string-non-empty, and string-printable
This option is no longer requiered because the implementation is now language independent.
Unknown values could cause type problems if we do not force the type for them.
5066dab to
6898cca
Compare
|
Superseded by #738 |
This adds the option --string-max-length to the string solver, this constrains all the strings to be smaller than the maximal length. This shouldn't affect much the performance of the decision procedure but may be necessary for obtaining concrete traces (when the --trace option is activated).
I also added the options --string-non-empty to constrain the nondet strings to have length at least 1 (to avoid obtaining counter-example that concatenates the empty string two times, which is not very interesting), and --string-printable to constrain strings to contain only characters in the range between (space, U+0020) and ~ (tilde, U+007E). making the example nicer than a sequence of null characters.
This also fixes a couple of bugs in the solver.
This corresponds to PR #652 on master