Documentation for decreases clauses#6332
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.
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.
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.
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.
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 <lopha@amazon.com>
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.