Skip to content
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

List prefix functions for Misc.Stdlib.List #2283

Merged
merged 3 commits into from Mar 6, 2019

Conversation

@mshinwell
Copy link
Contributor

commented Mar 5, 2019

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.

first_without_longest_common_prefix = l1;
second_without_longest_common_prefix = l2;
}
| elt1::l1_tail, elt2::l2_tail ->

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Mar 5, 2019

Contributor

What about using a when guard (when equal elt1 elt2), and a wildcard pattern for all other cases; this would avoid the code duplication.

| _::_, [] -> false
| [], _::_ -> true
| x1::t, x2::of_ ->
if equal x1 x2 then is_prefix ~equal t ~of_

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Mar 5, 2019

Contributor

equal x1 x2 && is_prefix ~equal t ~of_

@@ -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

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Mar 5, 2019

Contributor

The style used in this module is to have docstrings after definitions. It would seem better to stick to it.

Copy link
Contributor

left a 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 =

This comment has been minimized.

Copy link
@stedolan

stedolan Mar 5, 2019

Contributor

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 }
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@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.

@mshinwell mshinwell force-pushed the mshinwell:list_prefix branch from 222a24f to d2b62c9 Mar 6, 2019
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@alainfrisch Please see the new version and merge if you are happy, thanks.

@alainfrisch alainfrisch merged commit c975b15 into ocaml:trunk Mar 6, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

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).

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Really? I tend to think that if we're going to have a Changes file it should contain everything. (I don't buy the argument that users of compilerlibs will change their behaviour just based on what's in the Changes file, in particular.) In fact, as I was just saying to @dra27, I am hoping that we can find a solution to autogenerate the Changes file or equivalent -- the amount of wasted work due to merge conflicts is excessive.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

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.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@alainfrisch I suggest we take the Changes topic to caml-devel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.