Update error checking guidelines for clarity and to mention verify
#10
Conversation
gavin-norman-sociomantic
force-pushed
the
error-guide
branch
from
5782afc to
8986e6f
Compare
|
Added extra commits fixing smaller issues. |
gavin-norman-sociomantic
force-pushed
the
error-guide
branch
2 times, most recently
from
2c9a0e2 to
fa76a7f
Compare
Code/D/assert-vs-enforce.md
Outdated
| 3. `in` contracts (containing `assert`s) are fine to use in private methods/ | ||
| functions, as the test coverage of such code can be guaranteed within the scope | ||
| of a module. For non-private methods/functions, test coverage cannot be | ||
| guaranteed, so `in` contracts should be avoided. |
There was a problem hiding this comment.
I would put a different emphasis - private functions can only be called through some public one, which should have required verify check on its own before calling any private one.
There was a problem hiding this comment.
👍 that makes more sense. Updated.
`verify` did not exist when the guide was originally written.
gavin-norman-sociomantic
force-pushed
the
error-guide
branch
from
fa76a7f to
09ed454
Compare
|
Merging because I trust your work here and it's obvious that a second set of eyes was on it with @mihails-strasuns-sociomantic's comment. |
|
@dylan-cromwell-sociomantic for future reference, it'd be nice to wait for an approval review before merging. |
|
@gavin-norman-sociomantic apologies. Will do so for the future. Had thought the comment indicated this. |
|
@dylan-cromwell-sociomantic we normally wait for review by two involved devs before merging and generally try to give some time so that anyone can jump in and comment :) |
|
Yup, understood and makes perfect sense @mihails-strasuns-sociomantic! I will respect this going forward. Thanks for the heads up, you two! Any actions to take now then? |
|
No, it can be reviewed post-merge anyway and further PRs can be done if we find anything that needs fixing. I will review now for example :) |
|
Aha, ok, thanks @leandro-lucarella-sociomantic! |
| load. | ||
| 3. `in` contracts (containing `assert`s) should only be used on private methods/ | ||
| functions which are called via a public API including `verify`/`enforce` checks. | ||
| `in` contracts on non-private methods/functions should be avoided. |
There was a problem hiding this comment.
I would clarify why (I assume is because of https://issues.dlang.org/show_bug.cgi?id=6856, but not 100% sure)
| is being checked by a `verify` or `enforce` at some point in the code. If this | ||
| is not the case, convert the `assert` to a `verify` statement. | ||
|
|
||
| * All checks in `in` contracts should be converted to `verify` statements in the |
There was a problem hiding this comment.
I wonder if verify shouldn't just call assert() internally, so the program crashes in debug mode but let the application handle errors more gracefully in production builds.
verifydid not exist when the guide was originally written.