Gap syntaxtrees #114

sebasguts · 2019-03-16T12:44:37Z

No description provided.

fingolfin · 2019-03-18T09:43:39Z

@sebasguts is this up-to-date with your changes from yesterday?

I also had some more thoughts on the whole business last night we should discuss. One is that I think that we should never output "T_SEQ_STAT2", "T_SEQ_STAT3" etc., but only "T_SEQ_STAT"; then then when reassembling pieces, we should automatically use the appropriate variant. The same holds for the various 0ARGS, 1ARGS etc. calls, the loops and anything else involving these counts. Reason: While it of course makes it easier for us to produce and expect these tokens, it makes it much more complicated to manipulate these ASTs.

I've also been thinking about not produce T_SEQ_STAT at all, instead, it could just produce a list of statements. That would result in outputs that are easier to read for humans. And in a sense, T_SEQ_STAT is really an internal concept where I think it would make sense to abstract it away... I am not completely sure whether this is a good or bad idea, though... Anyway, it'd be easy to implement after applying the following patch:

@@ -99,9 +101,7 @@ static Obj SyntaxTreeCompiler(Expr expr)
 
     result = NewSyntaxTreeNode(comp.name);
 
-    comp.compile(result, expr);
-
-    return result;
+    return comp.compile(result, expr);
 }
 
 static Obj SyntaxTreeArgcompInt(UInt i)

With that, we can just throw away the record in the comp.compile function and instead return a list.

I also really want to come up with uniform terminology: As explained before, I dislike the current usage of "compile" for turning the built-in T_BODY (which also is a flattened AST) into a GAP manageable precord-of-precords-and-lists AST. If at all, I'd expect that "compile" would be the reverse direction. In this PR, you used "code" for the reverse direction, which suggest that we rename "compiler" to "decode". I could live with that, we should run it by @markuspf, too, though.

There were more things, but let's not take too many steps at once... :-)

sebasguts · 2019-03-18T11:52:57Z

@sebasguts is this up-to-date with your changes from yesterday?

Now it is

I also had some more thoughts on the whole business last night we should discuss. One is that I think that we should never output "T_SEQ_STAT2", "T_SEQ_STAT3" etc., but only "T_SEQ_STAT"; then then when reassembling pieces, we should automatically use the appropriate variant. The same holds for the various 0ARGS, 1ARGS etc. calls, the loops and anything else involving these counts. Reason: While it of course makes it easier for us to produce and expect these tokens, it makes it much more complicated to manipulate these ASTs.

Indeed, we can do that, with a fairly easy change I think. The interesting question is on how the read in function performs compared to the "regularly" typed in function, as it might be slower.

I also really want to come up with uniform terminology: As explained before, I dislike the current usage of "compile" for turning the built-in T_BODY (which also is a flattened AST) into a GAP manageable precord-of-precords-and-lists AST. If at all, I'd expect that "compile" would be the reverse direction. In this PR, you used "code" for the reverse direction, which suggest that we rename "compiler" to "decode". I could live with that, we should run it by @markuspf, too, though.

That is fine by me.

src/syntaxtree.c

src/code.c

src/syntaxtree.c

sebasguts · 2019-03-18T15:49:32Z

Should I make this a PR to gap-system, so we can continue discussing there? Or split this up?

sebasguts · 2019-03-18T22:51:08Z

I rebased the branch and added the float coders.

codecov · 2019-03-19T00:51:48Z

Codecov Report

Merging #114 into mh/syntaxtree will decrease coverage by 7.41%.
The diff coverage is 15.89%.

@@                Coverage Diff                @@
##           mh/syntaxtree     #114      +/-   ##
=================================================
- Coverage          69.12%   61.71%   -7.42%     
=================================================
  Files                633      633              
  Lines             305868   306032     +164     
=================================================
- Hits              211443   188871   -22572     
- Misses             94425   117161   +22736

Impacted Files	Coverage Δ
src/code.h	`86.11% <ø> (ø)`	⬆️
src/syntaxtree.c	`4.94% <0%> (-6.53%)`	⬇️
src/code.c	`90.66% <100%> (+0.04%)`	⬆️
src/intrprtr.c	`70.72% <100%> (+1.14%)`	⬆️
lib/meatauto.gi	`5.99% <0%> (-89.52%)`	⬇️
lib/nilpquot.gi	`11.11% <0%> (-88.89%)`	⬇️
lib/grpcompl.gi	`4.54% <0%> (-81.07%)`	⬇️
lib/reesmat.gi	`27.14% <0%> (-72.04%)`	⬇️
lib/randiso2.gi	`2.57% <0%> (-67.65%)`	⬇️
lib/filter.gi	`33.33% <0%> (-66.67%)`	⬇️
... and 286 more

fingolfin

Some comments. I've not yet read through all, but I figured it's better to give these to you already so you can get started :-)

src/syntaxtree.c

codecov-io · 2019-03-19T09:15:24Z

Codecov Report

Merging #114 into master will decrease coverage by 27.45%.
The diff coverage is 10.17%.

@@             Coverage Diff             @@
##           master     #114       +/-   ##
===========================================
- Coverage   69.17%   41.72%   -27.46%     
===========================================
  Files         633       99      -534     
  Lines      305868    40161   -265707     
===========================================
- Hits       211593    16756   -194837     
+ Misses      94275    23405    -70870

Impacted Files	Coverage Δ
src/code.h	`86.11% <ø> (ø)`	⬆️
src/code.c	`90.65% <100%> (+0.03%)`	⬆️
src/intrprtr.c	`69.57% <100%> (-1.15%)`	⬇️
src/syntaxtree.c	`6.34% <2.59%> (-5.14%)`	⬇️
src/system.c	`44.9% <0%> (-10.19%)`	⬇️
src/modules.c	`49.77% <0%> (-5.48%)`	⬇️
src/io.c	`41.58% <0%> (-5.03%)`	⬇️
src/vars.h	`80% <0%> (-3.34%)`	⬇️
src/objects.c	`50.99% <0%> (-3.16%)`	⬇️
src/gap.c	`41.68% <0%> (-2.11%)`	⬇️
... and 542 more

fingolfin

Some quick comments, I'll now read the full PR again.

fingolfin · 2019-03-19T14:28:11Z

src/syntaxtree.c

+    }
+    WRITE_EXPR(fl, 0, ix);
+    WRITE_EXPR(fl, 1, PushValue(value));
+    return fl;


This is 90% identical to CodeLazyFloatExpr. I wonder if we should just call that, and then use PopExpr. Or alternatively, extract the common code into a helper. I'd prefer that over exporting the super low-level getNextFloatExprNumber, which is a (problematic) implementation detail that I hope will go away at some point...

Either popping the expr, or just adding a parameter like for CodeFuncExprEnd and returning the expression. What would you prefer?

I added the additional parameter.

src/syntaxtree.c

fingolfin · 2019-03-19T14:44:31Z

src/syntaxtree.c

+static Expr SyntaxTreeCodeImmediateInteger(Obj node)
+{
+    Obj value = ELM_REC(node, RNamName("value"));
+    return INTEXPR_INT(Int_ObjInt(value));


So yeah, this is indeed unsafe because the resulting Int might be too big. So I'd switch back from Int_ObjInt to INT_INTOBJ, and instead insert an IS_INTOBJ check before (resp. use RequireSmallInt).

In an ideal world, though, I'd unify handling of T_INTEXPR and T_INT_EXPR: use only of them in the generated prec "tree"; then when coding back into a function, look at the value, and depending on whether it is immediate or not, output a T_INTEXPR or T_INT_EXPR... That would make it nicer to use. But doesn't have to happen in this PR!

src/syntaxtree.c

src/intrprtr.c

fingolfin · 2019-03-19T14:50:50Z

src/code.c

+    /* get the body of the function */
+    /* push an additional return-void-statement if necessary */
+    /* the function interpreters depend on each function ``returning'' */
+    if (nr == 0) {


The above formatting changes should be reverted.

fingolfin · 2019-03-19T14:50:57Z

src/code.c

-        {
+        PushStat(stat1);
+        if (TNUM_STAT(stat1) != T_RETURN_VOID &&
+            TNUM_STAT(stat1) != T_RETURN_OBJ) {


The above formatting changes should be reverted.

fingolfin · 2019-03-19T14:51:03Z

src/code.c

+    /* if the body is a long sequence, pack the other statements */
+    if (7 < nr) {
+        stat1 = PopSeqStat(nr - 6);
+        PushStat(stat1);


The above formatting changes should be reverted.

src/syntaxtree.c

fingolfin · 2019-03-19T14:56:04Z

tst/testinstall/syntaxtree.tst

@@ -1730,7 +1737,7 @@ rec(
 true

 # T_INFO
-gap> testit(function(x) Info(1, "test"); end);
+gap> testit(function(x) Info( 1, "test"); end);


fingolfin · 2019-03-19T14:58:18Z

tst/testinstall/syntaxtree.tst

@@ -2854,6 +2861,7 @@ rec(
  stats := rec(
      statements := [ rec(
              obj := rec(
+                  string := "1.0",
                  type := "T_FLOAT_EXPR_EAGER",
                  value := 1 ),


Hmm, how come the value is 1 here? I think that's wrong -- we probably are returning the index into the values array, instead of the value stored in it.

It is definitely the value, the code looks like this:

static Obj SyntaxTreeFloatEager(Obj result, Expr expr) { Obj value = GET_VALUE_FROM_CURRENT_BODY(READ_EXPR(expr, 0)); Obj string = GET_VALUE_FROM_CURRENT_BODY(READ_EXPR(expr, 1)); AssPRec(result, RNamName("value"), value); AssPRec(result, RNamName("string"), string); return result; }

OK, so it is the value, but again: how is that possible??? I still suspect something is wrong here...

OK, so the problem is the print method for floats:

gap> Print(1.); 1

src/syntaxtree.c

to indicate that the statement stack is complete and does not need any more additions.

sebasguts force-pushed the gap-syntaxtrees branch from 16dda94 to 3022308 Compare March 17, 2019 17:32

fingolfin force-pushed the mh/syntaxtree branch 2 times, most recently from 0112689 to 9dce3f6 Compare March 18, 2019 12:28

sebasguts force-pushed the gap-syntaxtrees branch from 92a1bd0 to 2f61be1 Compare March 18, 2019 14:13

fingolfin reviewed Mar 18, 2019

View reviewed changes

src/syntaxtree.c Outdated Show resolved Hide resolved

fingolfin reviewed Mar 18, 2019

View reviewed changes

src/syntaxtree.c Outdated Show resolved Hide resolved

fingolfin reviewed Mar 18, 2019

View reviewed changes

src/syntaxtree.c Outdated Show resolved Hide resolved

fingolfin reviewed Mar 18, 2019

View reviewed changes

src/code.c Outdated Show resolved Hide resolved

src/syntaxtree.c Outdated Show resolved Hide resolved

sebasguts force-pushed the gap-syntaxtrees branch 2 times, most recently from ee33756 to c4c6519 Compare March 18, 2019 15:40

sebasguts marked this pull request as ready for review March 18, 2019 15:40

fingolfin force-pushed the mh/syntaxtree branch 3 times, most recently from fdc0022 to 332e15f Compare March 18, 2019 16:41

sebasguts force-pushed the gap-syntaxtrees branch 2 times, most recently from 371055c to 583ffea Compare March 18, 2019 22:50

sebasguts force-pushed the gap-syntaxtrees branch from 583ffea to e476029 Compare March 19, 2019 00:51

fingolfin force-pushed the mh/syntaxtree branch from 332e15f to 62d24a6 Compare March 19, 2019 08:08

fingolfin requested changes Mar 19, 2019

View reviewed changes

WIP: add SyntaxTreeDefaultCoder

1933388

sebasguts force-pushed the gap-syntaxtrees branch 2 times, most recently from d50d976 to 2be1f97 Compare March 19, 2019 13:10

fingolfin changed the base branch from mh/syntaxtree to master March 19, 2019 14:37

sebasguts force-pushed the gap-syntaxtrees branch from 2be1f97 to 4fec202 Compare March 19, 2019 14:49

fingolfin reviewed Mar 19, 2019

View reviewed changes

sebasguts force-pushed the gap-syntaxtrees branch from 4fec202 to 56a4321 Compare March 19, 2019 14:54

fingolfin reviewed Mar 19, 2019

View reviewed changes

src/syntaxtree.c Outdated Show resolved Hide resolved

sebasguts added 2 commits March 19, 2019 16:35

Exposed functions from code.c in header

203b924

Added additional parameter to CodeFuncExprEnd

5a23554

to indicate that the statement stack is complete and does not need any more additions.

sebasguts force-pushed the gap-syntaxtrees branch 2 times, most recently from 81ef7ee to 2f82bf0 Compare March 19, 2019 15:51

sebasguts mentioned this pull request Mar 21, 2019

Pragmas #115

Closed

sebasguts added 3 commits March 21, 2019 17:54

Add additional parameter to CodeLazyFloatExpr

425d670

Finished first version of Syntax tree coder

4e5f52e

Added test function to compare syntax trees

9fa7002

sebasguts force-pushed the gap-syntaxtrees branch from 2f82bf0 to 9fa7002 Compare March 21, 2019 16:54

sebasguts mentioned this pull request Mar 21, 2019

Add a coder to reread syntaxtrees gap-system/gap#3371

Merged

fingolfin force-pushed the master branch from c873ea2 to d1b7da5 Compare August 1, 2022 08:38

fingolfin force-pushed the master branch from d60b304 to 999f835 Compare December 19, 2022 08:57

fingolfin closed this Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gap syntaxtrees #114

Gap syntaxtrees #114

sebasguts commented Mar 16, 2019

fingolfin commented Mar 18, 2019 •

edited

Loading

sebasguts commented Mar 18, 2019

sebasguts commented Mar 18, 2019

sebasguts commented Mar 18, 2019

codecov bot commented Mar 19, 2019 •

edited

Loading

fingolfin left a comment

codecov-io commented Mar 19, 2019 •

edited by codecov bot

Loading

fingolfin left a comment

fingolfin Mar 19, 2019

sebasguts Mar 19, 2019

sebasguts Mar 19, 2019

fingolfin Mar 19, 2019

fingolfin Mar 19, 2019

fingolfin Mar 19, 2019

fingolfin Mar 19, 2019

fingolfin Mar 19, 2019

fingolfin Mar 19, 2019

sebasguts Mar 19, 2019

fingolfin Mar 21, 2019

fingolfin Mar 21, 2019

Gap syntaxtrees #114

Gap syntaxtrees #114

Conversation

sebasguts commented Mar 16, 2019

fingolfin commented Mar 18, 2019 • edited Loading

sebasguts commented Mar 18, 2019

sebasguts commented Mar 18, 2019

sebasguts commented Mar 18, 2019

codecov bot commented Mar 19, 2019 • edited Loading

Codecov Report

fingolfin left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 19, 2019 • edited by codecov bot Loading

Codecov Report

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Mar 18, 2019 •

edited

Loading

codecov bot commented Mar 19, 2019 •

edited

Loading

codecov-io commented Mar 19, 2019 •

edited by codecov bot

Loading