Skip to content

Fix --string-non-empty option #2909

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 2, 2018
Merged

Fix --string-non-empty option #2909

merged 2 commits into from
Oct 2, 2018

Conversation

@romainbrenguier
Copy link
Contributor

@romainbrenguier romainbrenguier commented Sep 6, 2018

This was broken and the option had no effect

@romainbrenguier romainbrenguier force-pushed the bugfix/string-non-empty branch 2 times, most recently from d7f2bf9 to aef2b66 Compare September 6, 2018 12:26
tautschnig
tautschnig requested changes Sep 6, 2018
View reviewed changes
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please stop doing any command-line option parsing in the language front-end. Undocumented command-line options are a no-go.

jbmc/src/java_bytecode/java_bytecode_language.cpp
safe_string2size_t(cmd.get_value("max-nondet-string-length"));
}
if(cmd.isset("string-non-empty"))
object_factory_parameters.min_nondet_string_length = 1;
Copy link
Collaborator

@tautschnig tautschnig Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please not have more command-line parsing in the language front-end? See
#2907 and #2908. Let alone doing so via an undocumented option?

Copy link
Collaborator

@tautschnig tautschnig Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I remain unhappy it seems that #2908 is not currently progressing so I won't further block on this.

thk123
thk123 reviewed Sep 10, 2018
View reviewed changes
jbmc/src/java_bytecode/object_factory_parameters.h Outdated
/// Maximum value for the non-deterministically-chosen length of a string.
size_t max_nondet_string_length=MAX_NONDET_STRING_LENGTH;

/// Maximum value for the non-deterministically-chosen length of a string.
Copy link
Contributor

@thk123 thk123 Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 ⛏️ Typo: Minimum

@romainbrenguier romainbrenguier force-pushed the bugfix/string-non-empty branch 2 times, most recently from b25adc2 to 0e36b4d Compare October 2, 2018 08:28
@romainbrenguier romainbrenguier requested a review from thk123 October 2, 2018 08:29
@romainbrenguier romainbrenguier force-pushed the bugfix/string-non-empty branch from 0e36b4d to 2d7e636 Compare October 2, 2018 08:31
@romainbrenguier
Copy link
Contributor Author

romainbrenguier commented Oct 2, 2018

@thk123 @tautschnig this is ready for review

thk123
thk123 reviewed Oct 2, 2018
View reviewed changes
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test is missing one assertion status - please add. Also, any reason to have a binary flag rather than just a min-string-length option?

tautschnig
tautschnig approved these changes Oct 2, 2018
View reviewed changes
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok from my point of view, except the *.desc files as noted below. The where-to-parse-command-line-options is postponed.

jbmc/regression/jbmc-strings/string-non-empty-option/test.desc Outdated
assertion.* line 13 function java::Test.checkMinLength.*: FAILURE
assertion.* line 17 function java::Test.checkMinLength.*: FAILURE
assertion.* line 19 function java::Test.checkMinLength.*: FAILURE

Copy link
Collaborator

@tautschnig tautschnig Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blank lines here please, it confuses test runs on Windows. The same applies to the other *.desc files in this commit.

jbmc/src/java_bytecode/java_bytecode_language.cpp
safe_string2size_t(cmd.get_value("max-nondet-string-length"));
}
if(cmd.isset("string-non-empty"))
object_factory_parameters.min_nondet_string_length = 1;
Copy link
Collaborator

@tautschnig tautschnig Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I remain unhappy it seems that #2908 is not currently progressing so I won't further block on this.

@romainbrenguier romainbrenguier force-pushed the bugfix/string-non-empty branch from 2d7e636 to 72350c2 Compare October 2, 2018 09:20
@romainbrenguier romainbrenguier force-pushed the bugfix/string-non-empty branch from 72350c2 to cf123f0 Compare October 2, 2018 09:21
@romainbrenguier
Copy link
Contributor Author

romainbrenguier commented Oct 2, 2018

@thk123

Also, any reason to have a binary flag rather than just a min-string-length option?

No particular reason, I'm just fixing the previously existing feature that was broken, but it could be replaced by a min-string-length option in the future.

allredj
allredj reviewed Oct 2, 2018
View reviewed changes
Copy link
Contributor

@allredj allredj left a 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: 0e36b4d).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/86575976
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).

allredj
allredj reviewed Oct 2, 2018
View reviewed changes
Copy link
Contributor

@allredj allredj left a 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: 2d7e636).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/86578648
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).

allredj
allredj reviewed Oct 2, 2018
View reviewed changes
Copy link
Contributor

@allredj allredj left a 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: 72350c2).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/86579860
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).

allredj
allredj reviewed Oct 2, 2018
View reviewed changes
Copy link
Contributor

@allredj allredj left a 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: cf123f0).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/86580449

@romainbrenguier romainbrenguier merged commit 2c47e04 into diffblue:develop Oct 2, 2018
@romainbrenguier romainbrenguier deleted the bugfix/string-non-empty branch October 2, 2018 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tautschnig tautschnig tautschnig approved these changes

@peterschrammel peterschrammel peterschrammel approved these changes

@cesaro cesaro Awaiting requested review from cesaro

@cristina-david cristina-david Awaiting requested review from cristina-david

@mgudemann mgudemann Awaiting requested review from mgudemann

@NathanJPhillips NathanJPhillips Awaiting requested review from NathanJPhillips

@pkesseli pkesseli Awaiting requested review from pkesseli

@smowton smowton Awaiting requested review from smowton

@martin-cs martin-cs Awaiting requested review from martin-cs martin-cs is a code owner

+2 more reviewers

@allredj allredj allredj left review comments

@thk123 thk123 thk123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@romainbrenguier romainbrenguier

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@romainbrenguier @thk123 @tautschnig @peterschrammel @allredj