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

Add a coder to reread syntaxtrees #3371

Merged
merged 8 commits into from
May 7, 2019

Conversation

sebasguts
Copy link
Member

See here for the initial discussion and history

@wucas wucas added the gapdays2019-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2019-spring label Mar 22, 2019
@fingolfin
Copy link
Member

The first commit "WIP: add SyntaxTreeDefaultCoder" should be removed.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Overall this is looking close to be ready for merging, but I still left some comments.

src/code.c Show resolved Hide resolved
src/code.c Outdated Show resolved Hide resolved
src/code.h Outdated Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 27, 2019

Codecov Report

Merging #3371 into master will increase coverage by <.01%.
The diff coverage is 96.44%.

@@            Coverage Diff             @@
##           master    #3371      +/-   ##
==========================================
+ Coverage   85.33%   85.34%   +<.01%     
==========================================
  Files         699      699              
  Lines      345897   346082     +185     
==========================================
+ Hits       295181   295365     +184     
- Misses      50716    50717       +1
Impacted Files Coverage Δ
src/error.h 100% <ø> (ø) ⬆️
src/code.h 97.29% <ø> (ø) ⬆️
src/code.c 99.78% <100%> (ø) ⬆️
src/intrprtr.c 99.84% <100%> (ø) ⬆️
src/syntaxtree.c 96.12% <95.81%> (-0.75%) ⬇️
src/scanner.c 92.34% <0%> (ø) ⬆️
src/scanner.h 66.66% <0%> (ø) ⬆️
lib/maxsub.gi 70.43% <0%> (+0.13%) ⬆️
src/read.c 96.85% <0%> (+0.15%) ⬆️
src/stringobj.c 94.38% <0%> (+0.29%) ⬆️
... and 1 more

@sebasguts
Copy link
Member Author

@fingolfin I think I adressed all your comments. I also added sensible error messages for checking if a needed entry in a syntax tree node record is bound.

@sebasguts sebasguts force-pushed the sg/syntaxtree branch 2 times, most recently from 5438351 to 581bb00 Compare April 4, 2019 13:06
@coveralls
Copy link

coveralls commented Apr 4, 2019

Coverage Status

Coverage increased (+0.006%) to 85.152% when pulling 3c4614a on sebasguts:sg/syntaxtree into 1afd3f7 on gap-system:master.

@sebasguts
Copy link
Member Author

This PR is now based on #3370

src/syntaxtree.c Outdated Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
src/code.h Outdated Show resolved Hide resolved
src/code.h Outdated Show resolved Hide resolved
src/code.h Outdated Show resolved Hide resolved
src/code.h Outdated Show resolved Hide resolved
src/code.h Outdated Show resolved Hide resolved
src/code.h Outdated Show resolved Hide resolved
src/scanner.c Outdated Show resolved Hide resolved
src/scanner.h Outdated Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
@sebasguts sebasguts force-pushed the sg/syntaxtree branch 3 times, most recently from e0351b8 to 76c7e11 Compare April 11, 2019 15:15
@sebasguts
Copy link
Member Author

I think I addressed all your comments, and also added the checks we discussed today.

Should we try to add the context object, too, or would be a different PR a better place.

src/syntaxtree.c Outdated Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
src/syntaxtree.c Show resolved Hide resolved
src/syntaxtree.c Show resolved Hide resolved
src/syntaxtree.c Outdated Show resolved Hide resolved
@sebasguts
Copy link
Member Author

@fingolfin I think I adressed all of your comments

@sebasguts
Copy link
Member Author

@ChrisJefferson @fingolfin ping?

@ChrisJefferson
Copy link
Contributor

So, possibly stupid question, is this supposed to be complete, or progress towards a rereader?

I tried extending the test which tests SYNTAX_TREE on every function, by the following horrible patch:

@@ -25,11 +25,17 @@ gap> for n in NamesGVars() do
 >            v := ValueGlobal(n);
 >            if IsFunction(v) and not IsKernelFunction(v) then
 >                SYNTAX_TREE(v);
+>                        if not test_tree(v) then
+>                           Print("failed round trip: ",v,"\n");
+>                        fi;
 >            elif IsOperation(v) then
 >                for i in [1..6] do
 >                    for x in METHODS_OPERATION(v, i) do
 >                        if IsFunction(x) and not IsKernelFunction(v) then
 >                        SYNTAX_TREE(x);
+>                        if not test_tree(x) then
+>                           Print("failed round trip: ",v,"\n");
+>                        fi;
 >                        fi;
 >                    od; 
 >                od;

And GAP segfaults.

@sebasguts
Copy link
Member Author

@ChrisJefferson It does not work on operations, no. But I should really check that when calling SyntaxTree The segfault is really bad . . .

@sebasguts
Copy link
Member Author

Wait, this is a different issue. Sorry.

@ChrisJefferson
Copy link
Contributor

This is enough to upset it:

gap> f := function() return [,[]]; end;
function(  ) ... end
gap> SYNTAX_TREE_CODE(SYNTAX_TREE(f));

@sebasguts
Copy link
Member Author

I will fix that. I am not sure how that slipped. Thank you.

Anyway, there is a thing that I am aware of that does not work yet, namely HVAR references that do not point to the "read" function context anymore, but outside.

I plan to overcome this by adding a T_REF_VALUE (and T_ASS_VALUE) to GAP, so we can store those references not as HVARs anymore, but as values directly. However, this of course may change the overall semantics of a function . . .

@ChrisJefferson
Copy link
Contributor

(I don't know if this is reasonable), if in cases where SYNTAX_TREE_CODE failed it just returned 'fail', then we could obviously test for when it works, or returns fail, and then still loop over all the functions? (I don't know if that's reasonable given the way it works internally)

@sebasguts
Copy link
Member Author

@ChrisJefferson Currently, they throw an error, which I would guess is more reasonable. But I could do a no_error version for testing, if you would prefer?

The problem rereading those HVAR functions is that rereading them works, but their result is arbitrary, see here (which is worse than a segfault)):

gap> xx := (i -> ( j-> i + j ) )(3);
function( j ) ... end
gap> SYNTAX_TREE(SYNTAX_TREE_CODE(SYNTAX_TREE(xx)))=SYNTAX_TREE(xx);
true
gap> SYNTAX_TREE_CODE(SYNTAX_TREE(xx))(3);
Z(3)

@ChrisJefferson
Copy link
Contributor

I was just thinking about if it would be reasonable to merge something like the diff I made, to test a large set of functions. If we are still too far away from that, we can also merge the current partial code, and work on improving it later.

@sebasguts
Copy link
Member Author

No, absolutely reasonable. I will include it.

@sebasguts
Copy link
Member Author

@ChrisJefferson I added the full cycle test, and they pass
@fingolfin I made additional changes to CodeFuncExprEnd, please have a new look at the changes there.

> elif IsOperation(v) then
> for i in [1..6] do
> for x in METHODS_OPERATION(v, i) do
> if IsFunction(x) and not IsKernelFunction(v) then
> SYNTAX_TREE(x);
> if not test_tree(v) then
> Print("failed round trip: ",n,"\n");
Copy link
Member

Choose a reason for hiding this comment

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

Would be more useful like this:

Suggested change
> Print("failed round trip: ",n,"\n");
> Print("METHODS_OPERATION(", n, ",", i, ") failed round trip\n");

src/code.c Outdated
{
CodeReturnVoidWhichIsNotProfiled();
nr++;
while( 1 ){
Copy link
Member

Choose a reason for hiding this comment

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

Code formatting!

Suggested change
while( 1 ){
while (1) {

Copy link
Member

Choose a reason for hiding this comment

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

Also, this code really needs to be commented. Both what it is doing (see my suggestion below), but also more importantly, why it is doing it. Which I don't know right now...

Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't it suffice to insert the following into the existing code, to achieve the same effect?

  // find the last statement
  while (T_SEQ_STAT <= TNUM_STAT(stat1) && TNUM_STAT(stat1) <= T_SEQ_STAT7) {
    TNUM_STAT(stat1) <= T_SEQ_STAT7) {
    UInt size = SIZE_STAT(stat1) / sizeof(Stat);
    stat1 = READ_STAT(stat1, size - 1);
  }

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, logically, this does exactly the same, except it is not intertwined with the addition of the return void to the statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

About the why: The thing is that if we reread a syntax tree, the statements are already nested, i.e., if there are more than 7 statements in the body, the last one is already a new SEQ_STAT*. This ensures that the return; is only added if there is really no return in the last statement.

When a function is coded, the statements are packed into nested statements later.

I discovered this problem when I realized that "long" functions get an additional return;

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes topic: kernel labels May 7, 2019
@fingolfin fingolfin merged commit 5ecdc93 into gap-system:master May 7, 2019
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2019-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2019-spring kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants