List prefix functions for Misc.Stdlib.List #2283
Conversation
utils/misc.ml
Outdated
| first_without_longest_common_prefix = l1; | ||
| second_without_longest_common_prefix = l2; | ||
| } | ||
| | elt1::l1_tail, elt2::l2_tail -> |
There was a problem hiding this comment.
What about using a when guard (when equal elt1 elt2), and a wildcard pattern for all other cases; this would avoid the code duplication.
utils/misc.ml
Outdated
| | _::_, [] -> false | ||
| | [], _::_ -> true | ||
| | x1::t, x2::of_ -> | ||
| if equal x1 x2 then is_prefix ~equal t ~of_ |
There was a problem hiding this comment.
equal x1 x2 && is_prefix ~equal t ~of_
utils/misc.mli
Outdated
| @@ -116,6 +116,29 @@ module Stdlib : sig | |||
| (** [split_at n l] returns the pair [before, after] where [before] is | |||
| the [n] first elements of [l] and [after] the remaining ones. | |||
| If [l] has less than [n] elements, raises Invalid_argument. *) | |||
|
|
|||
| (** Returns [true] iff the given list, with respect to the given equality | |||
There was a problem hiding this comment.
The style used in this module is to have docstrings after definitions. It would seem better to stick to it.
There was a problem hiding this comment.
The code LGTM. Minor suggestions, which are not blocking.
My only concern is that we accumulate dead code in the compiler, with the hope that they become useful through later PRs. I understand that the idea is to split a bigger work into smaller, more easily reviewable chunks, but it would be better if this could be done through coherent chunks that add actual logic, not just helper functions. Reviewing code for internal helper functions without any context about the intended use cases seems a bit weird (but that's only my personal opinion, perhaps others are fully ok with it).
| } | ||
|
|
||
| let find_and_chop_longest_common_prefix ~equal ~first ~second = | ||
| let rec find_prefix ~longest_common_prefix_rev l1 l2 = |
There was a problem hiding this comment.
This function could be shorter and no less clear:
let rec find_prefix acc l1 l2 =
match l1, l2 with
| x :: l1, x' :: l2 when equal x x' ->
find_prefix (x :: acc) l1 l2
| l1, l2 ->
{ longest_common_prefix = List.rev acc;
first_without_longest_common_prefix = l1;
second_without_longest_common_prefix = l2 }
|
@alainfrisch Well it works both ways. Functions such as these (which should really be in the stdlib) should be standalone and completely independent of the context, in terms of semantics, so in some ways it's perhaps better not to see the calling context. As far as accumulating dead code goes, well, the only way to reasonably submit very large changes at the moment is from the bottom up. If we had better workflow management tools it might be possible to do otherwise, but we're not there yet. For these particular changes, the uses should appear in patches next week, depending on how review of existing pending GPRs goes. |
|
@alainfrisch Please see the new version and merge if you are happy, thanks. |
|
FWIW, I don't think a Changelog entry is needed for such PRs which don't impact users (we really don't want to advocate the use of those internal helper functions for users of compiler-libs). |
|
Really? I tend to think that if we're going to have a |
|
I don't see the point of an exhaustive, auto-generated Changes file. Commit messages and PR history are good enough for that. In my opinion, the goal of the Changes file is to inform users about changes which matter to them. I don't think that the addition of internal helpers functions, split in its own PR to help reviewing a subsequent PR, fall in this category. |
|
@alainfrisch I suggest we take the Changes topic to caml-devel. |
The work to improve the types of symbols requires some more prefix-related functions on lists, as presented here. I don't think we have anywhere to put unit tests for such things at present. These have been tested in the toplevel, which I think will suffice, nonetheless.