added parameter in count function #3

Anukriti12 · 2020-06-02T11:53:31Z

First PR: try_depth yet to be removed

On by default if +dev appears in VERSION and also fixed for MSVC.

lthls · 2020-06-02T12:02:04Z

(There's a huge number of commits in the diff because I forgot to keep my trunk branch in sync with the upstream one, sorry about that)

Anukriti12 · 2020-06-02T12:06:11Z

(There's a huge number of commits in the diff because I forgot to keep my trunk branch in sync with the upstream one, sorry about that)

I was figuring out whether there is pass by reference but was informed later through IRC channel that there isn't anything like pointers in OCaml. Will do more changes soon

lthls · 2020-06-02T12:11:31Z

Can you explain why you put Ident.Map.empty in some places instead of the depth argument from the caller ?

Apart from that, this looks like you've correctly added a new parameter to the count and count_default functions. The next step is to use this new parameter to carry the value that would otherwise be stored in try_depth.

I would suggest temporarily replacing the line let rec count depth = function with the two following lines:

let rec count depth lam =
  assert (depth = !try_depth);
  match lam with

which means that you will get an error during compilation if at some point the value for the new parameter and the old value kept in the reference disagree.

lthls · 2020-06-02T12:13:23Z

(It will also force the depth argument to have the same type as the contents of the try_depth reference, which should help getting rid of the extra Ident.Map.empty)

Anukriti1999 · 2020-06-02T12:20:03Z

Ohh this seems to be a nice approach, will add this. I added Ident.Map.empty for some debugging purpose while beginning but forgot to replace it.

More partial application warnings

This PR fixes an old bug in the interaction between [merge_constraint] and [Typedecl.transl_with_constraint], where variance (and now separability) are recomputed in an invalid type environment. See ocaml#9624 and the new tests.

(suggested by Leo White and Jacques Garrigue)

…riants (ocaml#9547)

fix an environment problem in merge_constraint

fix typo in ocaml#9569

Also: better explain how to encapsulate naked pointers in Abstract blocks.

Following review comments. Also: fix the assertion in the third val_of_typtr function.

… as others

@tailcall

We want to start allowing more information in the payload of [@tailcall] attributes (currently no payload is supported), for example we could consider using [@tailcall false] to ask the code generator to disable a tail call. A first required step in this direction is to use a custom datatype to represent the tail-call attribute, instead of a boolean. This is consistent with the other application-site attributes (inline_attribute, specialise_attribute, local_attribute), so it makes the code more regular -- but the change itself is boilerplate-y.

Eliminate MKEXE_ANSI from build system

…cision Document few Format quirks

matching: refactor the local control flow of the Unused exception

@tailcall

[minor] refactoring the datatype for the [@tailcall] attribute

…port ocamldoc: remove debugging facility

--enable-warn-error configure option

add explanation for variance flags

fix number and add examples for ocaml#6744

* Memprof: disable sampling when memprof is suspended. * Changes.

This patch removes casts between struct skiplist * and struct skipcell *, and removes the "layout compatibility" fields in skiplist that were there to enable these casts. The only algorithmic difference is that caml_skiplist_find is now "stop-at" (see discussion here), as this was slightly easier to write in the no-cast style.

* Remove temporary hack related to the Num library This is a follow-up to commit 3de0115 * Also do some cleanup in stdlib/Makefiile

* Introducing codefrag: a new runtime module to work with code fragments This module collects all the operations on code fragments performed in various places of the runtime systems. Applies both to bytecode and to native code. The implementation is based on skiplists, so that "lookup fragment by PC" and "lookup fragment by number" are efficient (logarithmic in the number of code fragments). "Lookup fragment by digest" remains linear-time. The new module also improves the handling of digests: now it is possible to mark a code fragment as "no digest" i.e. not marshal-able. * Use the new "codefrag" runtime module for marshaling and for the debugger interface Replace the previous handling of code fragments with calls to the functions provided by the "codefrag" runtime module.

lthls · 2020-06-12T08:59:42Z

The functional part of the task is done, good job.
I'll make a number of style comments that will help making the code easier to review for other people, but the first thing we want to do is to choose a name that is closer to the original try_depth name. The original name meant "depth of nested try...with expressions", and the "try" part is important.
One possibility is to reuse the original try_depth name. Now that the original reference has been removed, there is no ambiguity left, and it will make it very clear that it's used for the same purpose as the old reference.
I'll make a number of small inline comments about comments, white space, and stuff like that too.

lthls

I've pointed out all the places where I think some cleanup should be done. I think once you've done that we should think of submitting back the PR to the main repository, but that's not urgent.

lthls · 2020-06-12T09:01:21Z

lambda/simplif.ml

-
-  let try_depth = ref 0 in
+(* 
+  let try_depth = ref 0 in *)


Comment should be removed for the final version.

lthls · 2020-06-12T09:01:33Z

lambda/simplif.ml

+  let rec count depth = function
+  (* let rec count depth lam =
+    assert (depth = !try_depth);
+    match lam with *)


Comment should be removed for the final version.

lthls · 2020-06-12T09:02:03Z

lambda/simplif.ml

  | (Lvar _| Lconst _) -> ()
-  | Lapply ap -> count ap.ap_func; List.iter count ap.ap_args
-  | Lfunction {body} -> count body
+  | Lapply ap -> (count depth) ap.ap_func; List.iter (count depth) ap.ap_args


The first pair of parentheses is not needed.

lthls · 2020-06-12T09:02:45Z

lambda/simplif.ml

+        | _ -> count depth d; count depth d (* default will get replicated *)
      end
-  | Lstaticraise (i,ls) -> incr_exit i 1 !try_depth; List.iter count ls
+


There's an extra empty line here, that you should remove for consistency.

lthls · 2020-06-12T09:04:39Z

lambda/simplif.ml

  in
-  count lam;
-  assert(!try_depth = 0);
+


Extra empty line, might be better to remove it.

lthls · 2020-06-12T09:05:32Z

lambda/simplif.ml

  let subst = Hashtbl.create 17 in
-
-  let rec simplif = function
+  


There are two extra spaces on this line, most likely added by your editor; you should remove them (and keep the empty line).

lthls · 2020-06-12T09:06:05Z

lambda/simplif.ml

  | Lapply ap ->
-      Lapply{ap with ap_func = simplif ap.ap_func;
-                     ap_args = List.map simplif ap.ap_args}
+      Lapply{ap with ap_func = (simplif depth) ap.ap_func;


These parentheses are not needed, I would suggest removing them.

lthls · 2020-06-12T09:08:11Z

lambda/simplif.ml

-  | Lsequence(l1, l2) -> Lsequence(simplif l1, simplif l2)
-  | Lwhile(l1, l2) -> Lwhile(simplif l1, simplif l2)
+      let depth1 = depth +1 in
+      let l1 = simplif depth1 l1 in


Using simplif (depth + 1) l1 and removing the depth1 let-binding above probably better here.

lthls · 2020-06-15T09:19:28Z

I think we can now make a pull request on the main repository with your changes.
There's no guarantee that it will be merged, because it doesn't fix any bug or add any feature, but it could give you an idea of what kind of feedback to expect.
My own advice would be to submit a pull request but not spend too much time on it (in particular, you don't have to reply to every comment immediately), but I'll leave the choice to you.
I could also submit the PR on your behalf, if you prefer it this way.

Anukriti1999 · 2020-06-15T11:38:32Z

No, that's not a concern for me. I will be making the first PR today itself.

…l#13294) The toplevel printer detects cycles by keeping a hashtable of values that it has already traversed. However, some OCaml runtime types (at least bigarrays) may be partially uninitialized, and hashing them at arbitrary program points may read uninitialized memory. In particular, the OCaml testsuite fails when running with a memory-sanitizer enabled, as bigarray printing results in reads to uninitialized memory: ``` ==133712==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x4e6d11 in caml_ba_hash /var/home/edwin/git/ocaml/runtime/bigarray.c:486:45 #1 0x52474a in caml_hash /var/home/edwin/git/ocaml/runtime/hash.c:251:35 #2 0x599ebf in caml_interprete /var/home/edwin/git/ocaml/runtime/interp.c:1065:14 #3 0x5a909a in caml_main /var/home/edwin/git/ocaml/runtime/startup_byt.c:575:9 #4 0x540ccb in main /var/home/edwin/git/ocaml/runtime/main.c:37:3 #5 0x7f0910abb087 in __libc_start_call_main (/lib64/libc.so.6+0x2a087) (BuildId: 8f53abaad945a669f2bdcd25f471d80e077568ef) #6 0x7f0910abb14a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a14a) (BuildId: 8f53abaad945a669f2bdcd25f471d80e077568ef) #7 0x441804 in _start (/var/home/edwin/git/ocaml/runtime/ocamlrun+0x441804) (BuildId: 7a60eef57e1c2baf770bc38d10d6c227e60ead37) Uninitialized value was created by a heap allocation #0 0x47d306 in malloc (/var/home/edwin/git/ocaml/runtime/ocamlrun+0x47d306) (BuildId: 7a60eef57e1c2baf770bc38d10d6c227e60ead37) #1 0x4e7960 in caml_ba_alloc /var/home/edwin/git/ocaml/runtime/bigarray.c:246:12 #2 0x4e801f in caml_ba_create /var/home/edwin/git/ocaml/runtime/bigarray.c:673:10 #3 0x59b8fc in caml_interprete /var/home/edwin/git/ocaml/runtime/interp.c:1058:14 #4 0x5a909a in caml_main /var/home/edwin/git/ocaml/runtime/startup_byt.c:575:9 #5 0x540ccb in main /var/home/edwin/git/ocaml/runtime/main.c:37:3 #6 0x7f0910abb087 in __libc_start_call_main (/lib64/libc.so.6+0x2a087) (BuildId: 8f53abaad945a669f2bdcd25f471d80e077568ef) #7 0x7f0910abb14a in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x2a14a) (BuildId: 8f53abaad945a669f2bdcd25f471d80e077568ef) #8 0x441804 in _start (/var/home/edwin/git/ocaml/runtime/ocamlrun+0x441804) (BuildId: 7a60eef57e1c2baf770bc38d10d6c227e60ead37) SUMMARY: MemorySanitizer: use-of-uninitialized-value /var/home/edwin/git/ocaml/runtime/bigarray.c:486:45 in caml_ba_hash ``` The only use of hashing in genprintval is to avoid cycles, that is, it is only useful for OCaml values that contain other OCaml values (including possibly themselves). Bigarrays cannot introduce cycles, and they are always printed as "<abstr>" anyway. The present commit proposes to be more conservative in which values are hashed by the cycle detector to avoid this issue: we skip hashing any value with tag above No_scan_tag -- which may not contain any OCaml values. Suggested-by: Gabriel Scherer <gabriel.scherer@gmail.com> Signed-off-by: Edwin Török <edwin.torok@cloud.com> Co-authored-by: Edwin Török <edwin.torok@cloud.com>

garrigue and others added 5 commits May 25, 2020 19:04

add explanation for variance flags

1d27025

fix number and add examples for ocaml#6744

e0524b5

Add --enable-warn-error

75c9cd3

On by default if +dev appears in VERSION and also fixed for MSVC.

Rename $gcc_warnings to $cc_warnings in configure

9f9d806

More partial application warnings

fd1bb25

gasche and others added 20 commits June 2, 2020 17:22

Merge pull request ocaml#9560 from stedolan/more-partial

fd8188b

More partial application warnings

Fix the polymorphic recursion problem of ocaml#9603 (ocaml#9617)

ee20f5e

Fix load path ordering (ocaml#9611)

836d624

s/@raises/@raise

ca64723

add a testcase for 'with type <lhs> = <rhs>' scoping

45248a7

Typemod.merge_constraint: fix computation of the signature environment

8b2f64c

(suggested by Leo White and Jacques Garrigue)

fix typo in ocaml#9569

b280e17

Fix ocaml#7741: Failure to report escaping type variable (ocaml#9545)

13a081b

add PR number for ocaml#7741

97c3be2

Fix ocaml#7520: Odd behaviour of refutation cases with polymorphic va…

abb8db4

…riants (ocaml#9547)

Allow ',' at the start of OCAMLRUNPARAM

c0440d8

Changes

9ea385d

Merge pull request ocaml#9623 from gasche/unboxed-merge-constraint

84c87bb

fix an environment problem in merge_constraint

Merge pull request ocaml#9629 from hhugo/patch-3

9773de8

fix typo in ocaml#9569

FFI documentation: naked pointers are obsolete

608d166

Also: better explain how to encapsulate naked pointers in Abstract blocks.

Document alternate encodings for out-of-heap pointers

a22f6f5

Reword what will happen with pointers outside the heap

bea823a

Following review comments. Also: fix the assertion in the third val_of_typtr function.

minor Changes fix

42b64a7

Inria CI: also test LibBFD support on FreeBSD

e32c1b2

gasche and others added 16 commits June 8, 2020 15:39

[minor] printlambda: print the 'tailcall' attribute in the same style…

792deb1

… as others

Merge pull request ocaml#9285 from dra27/simplify-mkexe

d8f3273

Eliminate MKEXE_ANSI from build system

Remove two oldish scripts (ocaml#9656)

2e4d1ec

ocamldoc: remove debugging facility

433485b

Merge pull request ocaml#9618 from Octachron/format_documentation_pre…

a1e3b96

…cision Document few Format quirks

Merge pull request ocaml#9646 from gasche/rematch-simplify-unused

56f7500

matching: refactor the local control flow of the Unused exception

Merge pull request ocaml#9442 from gasche/tailcall-attribute-refactoring

b7509ca

[minor] refactoring the datatype for the [@tailcall] attribute

Merge pull request ocaml#9661 from shindere/ocamldoc-remove-debug-sup…

325a0ff

…port ocamldoc: remove debugging facility

Merge pull request ocaml#9625 from dra27/warn-error

2f38a52

--enable-warn-error configure option

Merge pull request ocaml#9602 from garrigue/document-variance-flags

6ea0fbd

add explanation for variance flags

Merge pull request ocaml#9612 from garrigue/pr6744ex

c241985

fix number and add examples for ocaml#6744

Memprof: disable sampling when memprof is suspended. (ocaml#9628)

7aad86f

* Memprof: disable sampling when memprof is suspended. * Changes.

Remove temporary installation hacks (ocaml#9659)

b94a377

* Remove temporary hack related to the Num library This is a follow-up to commit 3de0115 * Also do some cleanup in stdlib/Makefiile

Anukriti12 force-pushed the trunk branch from 176f231 to 08e58c8 Compare June 11, 2020 17:40

lthls reviewed Jun 12, 2020

View reviewed changes

Anukriti12 force-pushed the trunk branch 2 times, most recently from 5ba64b8 to 537920a Compare June 15, 2020 00:21

Removed last commit

accfbb5

Anukriti12 force-pushed the trunk branch from d4da7ec to accfbb5 Compare June 22, 2020 16:15

Anukriti12 added 3 commits June 22, 2020 22:24

made try_depth a labeled argument

642d854

tried to remove simplif files and added updatd version of linearize.ml

3ab4f86

removed previous_exit_labels and updated with changes

2e59846

lthls pushed a commit that referenced this pull request Oct 11, 2024

Unify mark and direction (#3)

9eadfb8

added parameter in count function #3

Are you sure you want to change the base?

added parameter in count function #3

Uh oh!

Conversation

Anukriti12 commented Jun 2, 2020

Uh oh!

lthls commented Jun 2, 2020

Uh oh!

Anukriti12 commented Jun 2, 2020

Uh oh!

lthls commented Jun 2, 2020

Uh oh!

lthls commented Jun 2, 2020

Uh oh!

Anukriti1999 commented Jun 2, 2020

Uh oh!

lthls commented Jun 12, 2020

Uh oh!

lthls left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lthls commented Jun 15, 2020

Uh oh!

Anukriti1999 commented Jun 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants