-
Notifications
You must be signed in to change notification settings - Fork 4
Handle common floats to string operations in the model and not in the string solver #23
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -531,11 +531,10 @@ public synchronized StringBuffer append(char c) { | |
| */ | ||
| @Override | ||
| public synchronized StringBuffer append(int i) { | ||
| // DIFFBLUE MODEL LIBRARY this is replaced internally | ||
| // toStringCache = null; | ||
| // super.append(i); | ||
| // return this; | ||
| return CProver.nondetWithoutNullForNotModelled(); | ||
| return append(String.valueOf(i)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏️ This isn't related to floats so should probably be in a separate commit.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. It's a follow-up from diffblue/cbmc#4633. I've split this into a separate commit. Other types should be coming later too, but at the moment I concentrated on the bugs that I've seen in our benchmarks, and am handling them one by one. |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -576,7 +575,7 @@ public synchronized StringBuffer append(float f) { | |
| // toStringCache = null; | ||
| // super.append(f); | ||
| // return this; | ||
| return CProver.nondetWithoutNullForNotModelled(); | ||
| return append(String.valueOf(f)); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -589,7 +588,7 @@ public synchronized StringBuffer append(double d) { | |
| // toStringCache = null; | ||
| // super.append(d); | ||
| // return this; | ||
| return CProver.nondetWithoutNullForNotModelled(); | ||
| return append(String.valueOf(d)); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
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 is the result of this method call different from just casting
dto typefloat?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.
In
doubleToFloatwe're ensuring that there's no loss of precision in the conversion, so that we convert the correct number to string. Say we haddouble x, then converted it todouble y, and then we converted that toString z.zis a string representation ofy, but not necessarily ofx. After a discussion with @romainbrenguier I went for thedoubleToFloatimplementation in order to make it more likely that the string representation ofxequals the string representation ofy.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 understand the precision problem but I don't understand how
doubleToFloatavoids this problem. Asfloathas fewer bits thandouble, wouldn't there always be a loss of precision? And can we be sure that for everydouble d, there is actually afloat fsuch thatd == (double) f;? Since there are strictly fewer floats than doubles this doesn't look right... and would mean that we sometimes can't get past theassumeindoubleToFloat.Maybe we could briefly discuss this in person? It's possible that I'm just missing something.
In any case, some tests in another repository would be great to see what this actually does/fixes.