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
refactoring: avoid long tuples in typing/typeclass.ml by using records with named fields #7927
Comments
Hi! I started on this issue by figuring out which functions need refactoring first. Then I put those functions and their large tuples in a table below to see overlaps. We can see that
Edit: btw, going through HACKING.adoc was very helpful. |
Answering 3, the benefits will be:
|
Going for the low-hanging fruit, that is the four functions you identified to use the exact same shape, and converting them into a shallow record (instead of trying to reorganize via nesting), sounds like a good first step to me.
For how to use the object-oriented parts of the language, see for example the Real World OCaml chapter on objects. For the implementation of the type system, For the theory behind it, it would be the academic article Objective ML: An effective object-oriented extension to ML (journal). I don't think any of these contain enough explanations to clarify the implementation, though. In any case, the nice thing with refactoring is that we don't really need to understand finely the code we are working with :-) |
@Octachron |
@Octachron @gasche
|
Generally, if a tuple is reused a lot of time, it may make sense to create a record for all elements of the tuple. For instance, here type class_env = { var: Env.t; meth: Env.t; parents:Env.t } Then the utility of the refactoring would depend on how well the record fits in the existing context. In particular, if it often needed to deconstruct and reconstruct the record often or not. |
@Octachron |
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
Note: this was in fact fixed in #8527. |
Original bug ID: 7927
Reporter: @gasche
Status: acknowledged (set by @gasche on 2019-02-20T15:43:53Z)
Resolution: open
Priority: low
Severity: feature
Category: typing
Tags: junior_job
Monitored by: @nojb
Bug description
typing/typeclass.ml uses long records with many components, only some of which are accessed each time. Example of not-so-nice-looking code includes:
Using records with named fields would improve the readability of this part of the codebase. Different tuples of different length are used in different places of the code, so three things may be useful:
(Note: one could think of using object types as extensible records here, but we are trying not to use objects directly in the compiler for bootstrapping reasons, so staying with less-flexible record types is the better idea.)
The text was updated successfully, but these errors were encountered: