-
Notifications
You must be signed in to change notification settings - Fork 25
Relaxed method resolution, based on sizes #271
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
Changes from 2 commits
43b8e7e
93b6df6
077a8c2
6068458
6ba1898
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,9 +11,10 @@ type t = | |||||||||||||
| hole : T.poly_fun option BRef.t; | ||||||||||||||
| (** Hole to be filled with method implementation *) | ||||||||||||||
|
|
||||||||||||||
| vset : Var.Set.t; | ||||||||||||||
| vset : int Var.Map.t; | ||||||||||||||
| (** Set of variables that cannot be used during method resolution. | ||||||||||||||
| This set is just passed as [~vset] to [ParamResolve.resolve_method]. | ||||||||||||||
| Do not use it directly. | ||||||||||||||
|
||||||||||||||
| (** Set of variables that cannot be used during method resolution. | |
| This set is just passed as [~vset] to [ParamResolve.resolve_method]. | |
| Do not use it directly. | |
| (** Map from variables to their size thresholds during method resolution. | |
| A variable can only be reused if the new usage has a strictly smaller size than the recorded value. | |
| This map is just passed as [~vset] to [ParamResolve.resolve_method]. Do not use it directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, that is correct. However, the actual representation of this "variable set" is an implementation detail of the ParamResolve module, but since the method constraints should be defined earlier, the actual type representation leaks to the public interface. I added the comment "Do not use it directly" to stress that this is an implementation detail, and the actual representation doesn't matter. I don't see any point in making the documentation more precise here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like the comments to at least acknowledge in any way that vset is a map. If I were to stumble upon this fragment while working on something else, I would probably feel very lost: a variable named vset, that comments repeatedly insist is a Set, but has type int Var.Map.t.
You are right that the comment stresses to "not use it directly", and links to the very place where the type and role of vset is explained, but I still think the code as it is now is needlessly arcane.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should change vset name to something that doesn't suggest a representation, like used_vars. Or, we can introduce another module, that provides some basic functionality related to these "sets". The type would remain abstract, and the code should be cleaner, but at the cost of introducing a new module. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be perfectly happy with renaming it to used_vars. I don't think it's worth introducing a new module for such a small part of what ultimately is a stopgap measure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The longer I look at this code, the more convinced I am that we should add a separate module for this "stopgap measure". I see at least two reasons to do that:
- data abstraction — I feel really bad that some internal data representation leaks, and pollutes types in other modules;
- code maintainability — this is simple data structure with a well-defined interface. However, I can imagine the situation when this module might grow in the future. For instance we can add command-line parameters to tune looping detection, like max depth proposed on the last meeting. Or when we will add type-classes we will face additional challenges that could be addressed there. If the implementation will stay in the
ParamResolvemodule we could end up with spaghetti code, that is hard to maintain.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| data B X = B of X | ||
| data P X Y = P of X, Y | ||
|
|
||
| data rec Shape = | ||
| | SU | ||
| | SB of Shape | ||
| | SP of Shape, Shape | ||
|
|
||
| method shape () = SU | ||
| method shape {X, method shape : X -> Shape} (B (x : X)) = | ||
| SB x.shape | ||
| method shape | ||
| { X, Y | ||
| , method shape : X -> Shape | ||
| , method shape : Y -> Shape | ||
| } (P (x : X) (y : Y)) = | ||
| SP x.shape y.shape | ||
|
|
||
| let _ = | ||
| (P (P (P (B (B ())) (B (P () ()))) (B (P () (B (P () ()))))) ()).shape |
Uh oh!
There was an error while loading. Please reload this page.