Skip to content

Just some tbl things. #1699

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

Merged
merged 10 commits into from
Jul 23, 2018
Merged

Just some tbl things. #1699

merged 10 commits into from
Jul 23, 2018

Conversation

Drup
Copy link
Contributor

@Drup Drup commented Apr 5, 2018

I always felt that the Tbl module was a grossslight violation of the DRY principle, so this patchset removes it.
On the way, I removed almost all the duplicated (String|Int)(Map|Set) in the compiler, and a few others.

This is my small contribution towards making the compiler feel like one code base, instead of the collision of five different ones. ;)

Details on Tbl

Tbl is basically a polymorphic Map module, using the polymorphic comparison with a few more hacks (such as Tbl.find_str which hardcodes the string comparison for speed. It is used for some string tables ... but not all of them). It has the exact same structure and semantics as Map, making it completely redundant.

On top of that, relying on the generic Tbl make local modifications fairly painful and invasive. For example, in the eliom patches, names are a couple of a string and a "location" (in eliom vocabulary) with a custom comparison function. With this patch, it can be implemented as a simple change in the implementation of Env.NameMap.

Patchset

The first 10 commits should be fairly consensual. They were mostly done through paste and replace.

The last 3 commits, particularly 484c79b , might be less consensual: Tbl made its way into the format for cmo files though the symbol tables for global variables and primitives. I replaced them by an Ident.Map (for global ids) and a StringMap (for primitives). I didn't bump the magic number (yet).

I'm not sure if I need a change entry for this.

Perfs

I don't expect any perf differences (the "hot" paths were already using Tbl.find_str). My non-scientific benchmark show a very minor speedup (around 1%, so probably noise). Is there some command to measure performance changes of the compiler on itself?

@Drup
Copy link
Contributor Author

Drup commented Apr 5, 2018

I do not understand travis' failure. It builds/tests fine locally and the various depend are up-to-date.

@Octachron
Copy link
Member

There is a .depend file hidden in ocamldoc/stdlib_non_prefixed, it seems that this the one responsible for the failure.

@mshinwell
Copy link
Contributor

I haven't got time to read this in detail right now, but will aim to do so in due course. I agree that cleanup here would be beneficial. This work seems related to some changes I am working on to integrate the utils/identifiable.ml* functionality directly into the standard library. I will write about that within the next few weeks.

@nojb
Copy link
Contributor

nojb commented Apr 6, 2018

I didn't bump the magic number (yet).

This is not necessary. Magic numbers will be bumped unconditionally for each release.

@dra27
Copy link
Member

dra27 commented Apr 6, 2018

At a glance, this looks good to me (although, if I may, I think the word gross should possibly only be used when describing one's own work!).

It's a basically internal change, so I agree it doesn't require a changes entry, but that does mean the work only gets credited in Git history, so feel free to add one if you'd like!

@Drup
Copy link
Contributor Author

Drup commented Apr 6, 2018

This work seems related to some changes I am working on to integrate the utils/identifiable.ml* functionality directly into the standard library.

Indeed! Making relevant data-structures in the compiler into Identifiable.S would also be very nice. I almost started doing that when I realized that Identifiable also include printing, and printing for the source/type language is not so trivial (the printers are defined somewhere else, and require lot's of machinery).

It's a basically internal change, so I agree it doesn't require a changes entry, but that does mean the work only gets credited in Git history, so feel free to add one if you'd like!

Ok, I will add one then.

@Drup Drup force-pushed the tbl branch 2 times, most recently from 8c26653 to 9ce89b2 Compare April 6, 2018 11:50
Copy link
Contributor

@mshinwell mshinwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made some comments and suggestions. This is good work.

@@ -16,12 +16,14 @@
(* Introduction of closures, uncurrying, recognition of direct calls *)

open Misc
open Numbers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a whole open, I would just use: module Int = Numbers.Int here.

open Asttypes
open Primitive
open Lambda
open Switch
open Clambda

module VarMap = Ident.Map
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is confusing ("ident"s versus "var"s). How about just replacing VarMap with Ident.Map throughout? That seems fine to me, and more clear in fact.

type t = int
let compare (x:t) y = compare x y
end)
module IntSet = Numbers.Int.Set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I would use module Int = Numbers.Int again, then this alias can be removed. Just use Int.Set instead of IntSet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -17,32 +17,33 @@
sequentialization. *)

open Misc
open Numbers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

@@ -19,6 +19,8 @@
open Asttypes
open Lambda

module VarMap = Ident.Map
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

typing/env.ml Outdated
@@ -150,7 +150,7 @@ end = struct

end

module PathMap = Map.Make(Path)
module NameMap = Misc.StringMap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Names of what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name, as in the individual parts in an Longindent.t. I would like to call them ident, but that clashes with the individual parts of a Path.t. I insist on having a local redefinition on that one, as it's the original motivation for this whole patch. But I agree, the name is bad.

@@ -165,6 +164,12 @@ let kind_of_field_desc = function
| Field_class _ -> "class"
| Field_classtype _ -> "class type"

(** Map indexed field names. Allow to preserve namespaces. *)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment isn't very clear---can you reword?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new one is :

(** Map indexed by both field types and names. 
    This avoids name clashes between different sorts of fields 
    such as values and types. *)

| Papply _ -> false
| Path.Pident _ -> true
| Path.Pdot(p, _, _) -> no_apply p
| Path.Papply _ -> false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, using disambiguation helps a lot for this kind of thing:

let can_alias env path =
  let rec no_apply (path : Path.t) =
    match path with
    | Pident _ -> true
    | Pdot(p, _, _) -> no_apply p
    | Papply _ -> false
  in
  ...

Copy link
Contributor Author

@Drup Drup Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this part of the compiler has warning 40 turned on (and fatal). I think changing default warnings in the implementation of the typechecker shouldn't be discussed in this PR.

let compare = compare
end
module Set = Set.Make(T)
module Map = Map.Make(T)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably use Identifiable.Make for this instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identifiable.Make require (two) printers and an equal function. I would prefer to leave that for later.

@@ -68,8 +68,10 @@ type error =
exception Error of Location.t * Env.t * error
exception Error_forward of Location.error

(** Map indexed by type variable names. *)
module VarMap = Misc.StringMap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example of why I recommended not having the VarMap alias up above in closure.ml: here the same alias means something completely different! (And it's not like it's a one-letter alias.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed all instances of VarMap outside of the typechecker and renamed that one as TyVarMap, which should make things clearer.

@mshinwell
Copy link
Contributor

Oh, and please add a Changes entry, in the "internal" section. This is a fairly large diff even if mostly straightforward.

@Drup Drup force-pushed the tbl branch 2 times, most recently from ce9a7a6 to bfa53aa Compare April 10, 2018 14:02
@Drup
Copy link
Contributor Author

Drup commented Apr 10, 2018

@mshinwell Thanks for the detailed review! I think I addressed most of your comments. I also fixed the change entry and rebased.

There are a few things (cleanup the 3 extended stdlibs, add even more identifiable everywhere) that would be very beneficial, but I would prefer to do them later, as the risk of conflict between this patchset and other changes is quite high.

@Drup
Copy link
Contributor Author

Drup commented Apr 10, 2018

While replacing ocamltest's StringMap and Sets, I realized the function stringset_of_string was unused. So I removed it and simply use aliases to Map.Make's output.

@Drup
Copy link
Contributor Author

Drup commented Jun 28, 2018

I rebased and added some fixed related to various recent additions.

I think I already addressed all of @mshinwell 's comment. Is there a remaining blocker?

I plan to squash things before the merge.

@Drup Drup force-pushed the tbl branch 2 times, most recently from b5120ed to fee2843 Compare June 29, 2018 00:26
@damiendoligez
Copy link
Member

You don't have to squash, just state that it should be squashed and we just use GitHub's "Squash and merge" option.

@gasche
Copy link
Member

gasche commented Jun 29, 2018

"Squash and merge" is generally terrible except for PRs that obviously fit in just one commit. If it's a series of nicely separated commits ("Remove Tbl in Foo" fits the pattern) plus some junk, you have to squash manually.

On the other hand, @Drup is making a mistake by not squashing right away. He is trying to save his time, but introduces complexity, delay, communication needs and mistake points in the process instead. One might even go as far as saying as he deserves to get his entry squashed!

@Drup
Copy link
Contributor Author

Drup commented Jun 29, 2018

@gasche I'm going to quote @mshinwell In another PR of mine:

Please ensure this isn't squashed (or similar) before merging as it will be difficult to review.

New commits are not squashed immediately with the old ones so that reviewers don't have to make high order diffs between stack of patchs. Unfortunately, we're not using Iron for OCaml PR reviews.

@gasche
Copy link
Member

gasche commented Jun 29, 2018

I would disagree with Mark on this one, but given that he is the one who should give you his seal of approval, it makes sense to please him.

@xavierleroy
Copy link
Contributor

xavierleroy commented Jun 29, 2018

"Squash and merge" is generally the right thing to do, because most PRs have an uninteresting history and uninteresting commit message, e.g. "Typos", "Going home", or "Add a Changes entry". Indeed this PR has one commit titled "Really?!" and another titled "Depend". I don't want to see this nor the 18 intermediate states of this PR in OCaml's commit log. I want to see one entry in the log with one message that summarizes what the PR does in its final state.

@dra27
Copy link
Member

dra27 commented Jun 30, 2018

If you want to tidy the history, then I'm sure @mshinwell can be persuaded to merge a clean commit history! However, before this is merged, please could check-typo violations be dealt with:

./asmcomp/selectgen.ml:798.81: [long-line] line is over 80 columns
./asmcomp/selectgen.ml:1129.81: [long-line] line is over 80 columns
./bytecomp/simplif.ml:419.81: [long-line] line is over 80 columns
./bytecomp/symtable.ml:43.3: [white-at-eol] whitespace at end of line
./bytecomp/symtable.ml:62.3: [white-at-eol] whitespace at end of line
./tools/dumpobj.ml:533.81: [long-line] line is over 80 columns
./typing/includemod.ml:167.48: [white-at-eol] whitespace at end of line
./typing/includemod.ml:168.64: [white-at-eol] whitespace at end of line
./typing/includemod.ml:319.81: [long-line] line is over 80 columns
./utils/misc.ml:180.40: [white-at-eol] whitespace at end of line
./utils/misc.mli:105.7: [white-at-eol] whitespace at end of line

@damiendoligez
Copy link
Member

It is generally a bad idea to squash before the end because the commits get out of sync with the discussion and it becomes hard to make sense of them.

@Drup
Copy link
Contributor Author

Drup commented Jul 4, 2018

@dra27 All fixed, and rebased on top of trunk to take advantage of the nice typocleaning.

@dra27
Copy link
Member

dra27 commented Jul 5, 2018

Thanks for the check-typo loveliness 🙂

@Drup
Copy link
Contributor Author

Drup commented Jul 16, 2018

4.07 is released, the world cup is finished. @mshinwell, can I get a review? :)

@mshinwell
Copy link
Contributor

Please add a comment in env.ml on the "NameMap" definition to say what "Name" means here, since we can't come up with anything better. OK to merge with that change. Thanks for taking the time to do this, it seems like a real improvement.

@Drup
Copy link
Contributor Author

Drup commented Jul 23, 2018

Done, rebased, and history somewhat cleaned up.

@Drup
Copy link
Contributor Author

Drup commented Jul 23, 2018

CI is green.

@gasche
Copy link
Member

gasche commented Jul 23, 2018

I'm a bit worried by the last commit fixing a test failure. This means that the testsuite fails if I am bisecting and end in the middle of your patchset, right? Could you maybe fixup this commit at the first point where the change is required, to maintain the invariant that intermediate states are clean?

@mshinwell
Copy link
Contributor

I was planning to squash the whole lot; I'm not sure any intermediate state is useful here.

@Drup
Copy link
Contributor Author

Drup commented Jul 23, 2018

@mshinwell Do as you wish. :p

@mshinwell mshinwell merged commit 1be47bf into ocaml:trunk Jul 23, 2018
@Drup Drup deleted the tbl branch July 28, 2018 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants