[matching.ml cleanup] abstract default environment#8850
Conversation
|
|
||
| val pop_column : t -> t | ||
|
|
||
| val pop_compat : pattern -> t -> t |
There was a problem hiding this comment.
Isn't there a contradiction between the pop_column function, which remove one column from every matrix, and pop_compat, which remove one column from compatible rows but also remove incompatible rows? Maybe keeping specialize_compat as a name would be clearer here.
Similarly, pop and pop_column have a slightly different meaning for their common pop root since only pop returns the dropped leftmost element(s), but this sounds like a minor discrepancy at most.
There was a problem hiding this comment.
Yes, I kept the names that were already there, but I guess just specialize would be a decent name.
There is also val pop : ... which pops the first matrix from the list... Perhaps pop_column should become pop_first_column_of_matrices?
There was a problem hiding this comment.
Another possibility might be specialize_for_var to describe the use case rather than the implementation?
There was a problem hiding this comment.
pop_compat is the composition of pop_column and filter_compat (or guard_compat). I don't think that specialize is more informative as a name, and I'm not fond of the longer names proposed (pop_first_column_of_matrices should be map_rows pop); maybe just implement filter_compat and write the composition by hand? (It's one more traversal of the whatever, but it doesn't actually matter much.)
There was a problem hiding this comment.
@trefis and myself have discussed better interfaces for matchers, in particular pattern -> pattern list (where NoMatch is the empty list and OrPat returns a list of explosions) and a version where OrPat is removed (replaced by generic handling of or-patterns in the specialization code), allowing pattern -> pattern option. I think that at this point it may be possible to keep only specialize in the module, and write the matchers nicely in the client code, obviating the need for those API naming discussions.
In the meantime, my inclination would be to keep the names as they currently are to keep the refactoring train going.
There was a problem hiding this comment.
It is most probably a good idea to let the refactoring train go on. And, maybe, or not, I will have a look at the end of the refactorization tunnel to see if those names are still here.
Octachron
left a comment
There was a problem hiding this comment.
The new comments are really nice. The new module clarifies the code structure, but the names are maybe a bit too implementation-specific.
For instance Default_env.pop_column sounds more like a description of the function's action on the underlying implementation that a meaningful action on a self-contained default environment concept. But this not may be the right PR to discuss that kind of issues.
|
I don't have a definitive mind made on the question, but I think that it's good to keep "operational" names sometimes if they coincide with how the people writing the code think about the implementation. Luc really pops columns, and it's also the way most matrix-decomposition algorithms (for both compilation and analysis) are expressed and thought about. |
|
A default environment Two other ways to say similar things:
|
f4666a5 to
01099da
Compare
|
I have updated the PR following comments (I ended up not renaming I will merge once the CI is green. |
The bug comes from <ocaml#8850> that we wrote and got merged last August. It breaks specialization of default matrices, so in theory it should not lead to wrong-code production, but because it can create default matrices of distinct arity in the same environment it should be possible to observe assertion failures in code hitting the bug. The bug was made possible and in fact easy (and also uncatchable by human eyes, including the almost-super-human-reviewer of ocaml#8850) by a dubious stylistic choice made in 842eb76 in 2002, where `rem` sometimes means the not-yet-filtered remainder and, only in one non-idiomatic case, is the filtered remainder. I found the bug while upgrading the code to use better types. The input of specialize_matrix has non_empty rows, but the output may be empty -- and this code becomes ill-typed when input and output have distinct types. Yay typing.
This PR:
Note: I believe the code change in the 1st commit to be correct, but the comment added in that commit should be double checked.