-
Notifications
You must be signed in to change notification settings - Fork 274
Documentation for decreases clauses #6332
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
1bf954c
to
fd03dcc
Compare
Codecov Report
@@ Coverage Diff @@
## develop #6332 +/- ##
========================================
Coverage 75.89% 75.89%
========================================
Files 1515 1515
Lines 163972 163972
========================================
Hits 124444 124444
Misses 39528 39528 Continue to review full report at Codecov.
|
16a24ee
to
74525c1
Compare
@SaswatPadhi can we close #6149 after merging this PR? The only initial documentation missing will be assigns clause for loops, correct? |
74525c1
to
2e40735
Compare
2e40735
to
13f0022
Compare
I think we can close it after this + assigns clause documentation is merged. I would push the assigns clause documentation to #6249 |
0dd6555
to
f46d60a
Compare
I have addressed all prior comments and fixed some build issues (with the new binary_search regression test). Keeping this PR open for one final review from @jimgrundy. |
|| (a_1 == b_1 && a_2 < b_2) | ||
|| ... | ||
|| (a_1 == b_1 && ... && a_n < b_n) | ||
``` |
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 would suggest cutting the material after "strict lexicographic comparison". Maybe hyperlink off "lexicographic comparison" to the article you linked earlier, and you can skip the explanation of how you implement a lexicographic compare. If people know what one is already they don't need to see this code. If they don't, they might be better off reading the code. But, if you like it, then I'm OK with it.
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.
Okay, I could remove it. I wasn't sure if end users would be familiar with exactly what lexicographic comparison on ordered tuples would mean. I didn't want to point them to research papers just for that, but I guess they could Google it if they are unsure :)
if(size <= 0 || buf == NULL) return NOT_FOUND; | ||
|
||
long long lb = 0, ub = size - 1; | ||
long long mid = (lb + ub) / 2; |
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 works because we use "long long" values to avoid overflow. But, we can do the computation another way and avoid overflow while using fewer bits, which might give better solver performance. Take a look at this article for a nice clear way to express the algorithm without the extra bits: Extra, Extra - Read All About It: Nearly All Binary Searches and Mergesorts are Broken. Now, since the use of int for the arguments and long long for the computation should mean this is correct (are there machines for which int is the same size as long long?) then I'm fine with leaving this as is, but I think you might like the version in this article better anyway
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! :D Great point!
Now, since the use of int for the arguments and long long for the computation should mean this is correct (are there machines for which int is the same size as long long?)
It's correct (verified) only on 64-bit Windows and Linux machines -- the CBMC CI machines & my machine.
I originally had long
, and that failed because on Windows int
== long
, apparently! Then I changed to long long
and both Windows + Linux were happy. But that's still fragile -- even long long
could be the same as int
on some platform.
I implemented the change. The verification time with MiniSAT remains the same (~2.7s), but the number of variables in the SAT formula goes from ~12900 down to ~7700.
ac982ca
to
5cc40e9
Compare
Co-authored-by: Long Pham <[email protected]>
5cc40e9
to
12b22c0
Compare
Related to #6149.
Thanks to @LongPham7 for an initial draft!
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).My commit message includes data points confirming performance improvements (if claimed).White-space or formatting changes outside the feature-related changed lines are in commits of their own.