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
Assertions cleanup #1097
Assertions cleanup #1097
Conversation
Assertions hanging to the right of the page are one of @damiendoligez's mannerisms.
Preserved, of course, like all mannerisms from great masters. |
I wondered which of the two asserts below was being meant. The "3 lines" measure was broken by ddd51af, and the relevant assert is (as could be expected) the first one, that is the |
df7671b
to
e2f6e02
Compare
Gabriel Scherer (2017/03/10 10:35 -0800):
> in `byterun/intern.c` [...] maybe "3 lines down" could be replaced by "below"?
I wondered which of the two asserts below was being meant. The "3
lines" measure was broken by ddd51af,
and the relevant assert is (as could be expected) the first one, that
is the `intern_color` Assert. I would rephrase this as "the
intern_color Assert below".
Done, thanks. It now says "and break the intern_color assertion below".
Xavier Leroy (2017/03/10 10:27 -0800):
> does anybody know why the assertions have not been aligned ?
Assertions hanging to the right of the page are one of
@damiendoligez's
mannerisms.
> Does this need to be preserved or changed?
Preserved, of course, like all mannerisms from great masters.
Okay :)
|
e2f6e02
to
fc924ed
Compare
Just rebased on latest trunk. Does it look okay to be merged, now? |
Replace a few occurrences of assert and many occurrences of Assert by CAMLassert. In asmrun/spacetime_snapshot.c, the line Assert(0); /* unreachable */ in the function caml_spacetime_shape_table has been removed rather than replaced because the caml_failwith function called just above has the noreturn attribute which should be enough to keep compilers happy. The same holds in the function caml_spacetime_only_works_for_native_code defined in byterun/spacetime.c
fc924ed
to
adcb449
Compare
Nah, I've grown out of this one... |
Damien Doligez (2017/03/14 09:04 -0700):
> Preserved, of course, like all mannerisms from great masters.
Nah, I've grown out of this one...
Pretty cool, cause (1) it's not so uniform accross OCaml's source
code and (2) it's not that convenient from an a11y (accessibility, like
i18n for internationalization) perspective: when going through the code
with down arrow and cursor at the beginning of lines, if an assertion
starts at character 41 or more, I'll see what looks like a blank line
and completely miss it. Unless, of course, I am in search for it, in
which case I'll scroll and see it.
|
This is indeed a very good reason to remove it. |
This is a follow-up to the discussion that took place in PR #1097.
Damien Doligez (2017/03/15 09:13 -0700):
> (2) it's not that convenient from an a11y (accessibility, like
i18n for internationalization) perspective
This is indeed a very good reason to remove it.
Done in commit 0aab402 which has just
been pushed to trunk.
|
Replace a few occurrences of assert and many occurrences of Assert
by CAMLassert.
For multi-line assertions, an effort has been made to keep the code
both aligned correctly and beyond the 80-characters-per-line limit.
Sighted input would be welcome to make sure this has actually been achieved.
Also, in byterun/caml/weak.h, function caml_ephe_clean, does
anybody know why the assertions have not been aligned with
header_t hd; ?
Does this need to be preserved or changed?
Similar question for byterun/compact.c, byterun/minor_gc.c, byterun/misc.c and
byterun/freelist.c.
In this file, does the big comment with all the assertions in it
need to be preserved?
In byterun/intern.c, see the comment saying: "do not do the urgent_gc
check here because it might darken intern_block into gray and break
the Assert 3 lines down.": maybe "3 lines down" could be replaced by "below"?