Improve cppo usage - #105
Conversation
Gather most of the uses in Utils.Compat, in dedicated functions (either conversions or getters). Sort the branches in version descending order. If the conditional branches are close enough, do not repeat the bounds. If the #else stands out enough, do not convert it into #elif. If all the versions should be accounted without #else, add an #else that throws an #error.
Instead of systematically returning an option, they now return a result, and a dedicated error gadt `'a invalid_arg` is introduced. In addition, getters have a `*_exn` variant to directly unwrap the result when the argument is already known to have the right shape.
kit-ty-kate
left a comment
kit-ty-kate
left a comment
There was a problem hiding this comment.
looks good overall. Note that if you want to reduce the diff you can keep the ordering by instead doing:
#if OCAML_VERSION < (5, 3, 0)
#elif OCAML_VERSION < (5.4.0)
#else
#endif
| | _ -> Result.error "No implementation found in cmt_infos" | ||
|
|
||
|
|
||
| #elif OCAML_VERSION >= (4, 14, 0) && OCAML_VERSION < (5, 3, 0) |
There was a problem hiding this comment.
the second clause is redundant
| #elif OCAML_VERSION >= (4, 14, 0) && OCAML_VERSION < (5, 3, 0) | |
| #elif OCAML_VERSION >= (4, 14, 0) |
There was a problem hiding this comment.
Yes. This is on purpose as a reminder because the previous condition is 90 lines above.
I find it more pleasant to have the redundancy and all the information quickly available, than having to remember the previous bound or jumping back and forth to be reminded.
| #endif | ||
| process_params params; | ||
| process_body body | ||
| #elif OCAML_VERSION >= (4, 14, 0) && OCAML_VERSION < (5, 2, 0) |
There was a problem hiding this comment.
| #elif OCAML_VERSION >= (4, 14, 0) && OCAML_VERSION < (5, 2, 0) | |
| #elif OCAML_VERSION >= (4, 14, 0) |
| #else | ||
| #error "unsupported version" |
There was a problem hiding this comment.
is this preferred over having just a #else case instead of the elif right above? Does this mean the code would compile on 4.13?
There was a problem hiding this comment.
I prefer this when the conditions are far apart (in this case the #if and #elif are 25 lines apart). This way, when reaching the #elif, its condition provides all the necessary information to read its branch without having to remember the previous condition.
The #else is a safeguard against forgetting a version.
However, when the conditions are sufficiently localized for the previous condition to be in sight, I feel comfortable using an #else.
There is no plan to support OCaml < 4.14 at the moment.
The code would actually not compile because preprocessing would fail with the "unsupported version" error message (and probably for other reasons).
2 participants
Gather most of the uses in
Utils.Compat, in dedicated functions (either conversions or getters).Sort the branches in version descending order.
Iff the conditional branches are close enough, do not repeat the bounds, and iff the #else is close enough the the previous condition, do not convert it into #elif.
If all the versions are accounted without #else, add an #else that throws an #error.