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

Show files/location for compiled functions #755

Merged
merged 1 commit into from
May 25, 2016

Conversation

ChrisJefferson
Copy link
Contributor

This patch makes GAP show information about where kernel or compiled functions come from. This is best illustrated by an example! RETURN_FIRST is a kernel function, RunImmediateMethods and InstallMethod are both from oper1.g, which is compiled.

gap> Display(RETURN_FIRST);
function ( object... )
    <<kernel or compiled code from src/gap.c:RETURN_FIRST>>
end
gap> Display(RunImmediateMethods);
function ( <<arg-1>>, <<arg-2>> )
    <<kernel or compiled code from GAPROOT/lib/oper1.g:26>>
end
gap> Display(InstallMethod);
function ( <<arg-1>>... )
    <<kernel or compiled code from GAPROOT/lib/oper1.g:282>>
end

For compiled GAP code, we already knew this info, we just weren't displaying it. For C functions, I use the 'cookie' value, which is usually set by convention to filename:Cfunction. We should probably document that we are now using the cookie information for this purpose (if the cookie doesn't have a : in it, we just ignore it and don't try to extract filename/function information).

@olexandr-konovalov
Copy link
Member

Looks nice. Is there a way now to distinguish between the kernel and compiled code by its location and show e.g.

    <<kernel code from src/gap.c:RETURN_FIRST>>

and

    <<compiled code from GAPROOT/lib/oper1.g:26>>

?

@ChrisJefferson
Copy link
Contributor Author

Yes, but I'm just trying to think of a neat way to do it, rather than something hacky, or wasting bits.

@ChrisJefferson
Copy link
Contributor Author

Now tweaked (also added some kernel comments in case anyone else ever comes and reads this code). Decided to write Compiled GAP code, so it's clear this is GAP code (obviously kernel C code is still compiled!)

gap> Display(RETURN_FIRST);
function ( object... )
    <<kernel code from src/gap.c:RETURN_FIRST>>
end
gap> Display(RunImmediateMethods);
function ( <<arg-1>>, <<arg-2>> )
    <<compiled GAP code from GAPROOT/lib/oper1.g:26>>
end
gap> Display(InstallMethod);
function ( <<arg-1>>... )
    <<compiled GAP code from GAPROOT/lib/oper1.g:282>>
end

@ChrisJefferson
Copy link
Contributor Author

ChrisJefferson commented Apr 19, 2016

Don't merge this -- something weird going on : EDIT, nope, weirdness has always been there, ignore this.

@olexandr-konovalov olexandr-konovalov added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) and removed do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) labels May 12, 2016
ArgStringToList( tab[i].args ),
tab[i].handler );
SetupFuncInfo( func, tab[i].cookie );
AssGVar( gvar, func );
MakeReadOnlyGVar( gvar );
Copy link
Member

Choose a reason for hiding this comment

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

Note that some kernel extensions in packages do not use InitGVarFuncsFromTable.

So I wonder: Will it be safe to use function for which SetupFuncInfo has not been invoked? I assume yes, but I wanted to double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Such functions will continue to show no position, as they always did.

@olexandr-konovalov
Copy link
Member

For the record, if merged, this would close issue #745.

@ChrisJefferson
Copy link
Contributor Author

This code has had a general spring clean, is now (hopefully) better defined, don't abuse the same variable for multiple uses.

@markuspf markuspf merged commit eaec3ab into gap-system:master May 25, 2016
@olexandr-konovalov
Copy link
Member

@ChrisJefferson I was surprised by the following side-effect when all packages are loaded:

########> Diff in /Volumes/hudson-fs/workspace/GAP-dev/GAPCOPTS/64build/GAPGMP\
/gmp/GAPTARGET/install/label/fruitloop/GAP-git-snapshot/tst/testinstall/vararg\
s.tst:84
# Input is:
Display(RunImmediateMethods);
# Expected output:
function ( <<arg-1>>, <<arg-2>> )
   <<compiled GAP code from GAPROOT/lib/oper1.g:26>>
end
# But found:
function ( obj, bitlist )
   if
    HasSomethingToDo( obj ) and CanHaveAToDoList( obj ) 
       and TODO_LISTS.activated  then
       ProcessToDoList_Real( obj, bitlist );
   fi;
   ORIG_RunImmediateMethods( obj, bitlist );
   return;
end
########

because ToolsForHomalg has the following lines:

###### CRUCIAL:
###### This is a HACK given by Max H.
###### Maybe we can replace this later.
###### It also might slow down the whole system.

ORIG_RunImmediateMethods := RunImmediateMethods;
MakeReadWriteGlobal("RunImmediateMethods");
NEW_RunImmediateMethods := function( obj, bitlist )
                              if HasSomethingToDo( obj ) and CanHaveAToDoList( obj ) and TODO_LISTS.activated then
                                  ProcessToDoList_Real( obj, bitlist );
                              fi;
                              ORIG_RunImmediateMethods( obj, bitlist );
                           end;
RunImmediateMethods:=NEW_RunImmediateMethods;
MakeReadOnlyGlobal("RunImmediateMethods");

Does the failing command from the test file check anything special, like 2-argument function? Otherwise, it may be removed since next command from the test files looks very similar and works.

@ChrisJefferson
Copy link
Contributor Author

It is testing something useful (non-variadic printing). Could you try replacing it with:

Print(INSTALL_METHOD);
function ( <<arg-1>>, <<arg-2>> )
    <<compiled GAP code from GAPROOT/lib/oper1.g:322>>
end

Which should hopefully work (I think I've got the number right).

@ChrisJefferson ChrisJefferson deleted the compiled-info branch May 25, 2016 22:21
@olexandr-konovalov
Copy link
Member

Is there a new line missing somewhere? This is what I am getting:

########> Diff in /Users/alexk/GITREPS/gap/tst/testinstall/varargs.tst:84
# Input is:
Print(INSTALL_METHOD);
# Expected output:
function ( <<arg-1>>, <<arg-2>> )
    <<compiled GAP code from GAPROOT/lib/oper1.g:322>>
end
# But found:
function ( <<arg-1>>, <<arg-2>> )
    <<compiled GAP code from GAPROOT/lib/oper1.g:322>>
end########
varargs.tst                0             60   ( next ~493 sec )

@ChrisJefferson
Copy link
Contributor Author

Bah, I hate that stupid lack of newline at the end of functions. Should get around to fixing it.

Print(INSTALL_METHOD,"\n"); should fix it..

olexandr-konovalov pushed a commit that referenced this pull request May 26, 2016
This avoids diffs when ToolsForHomalg package is loaded (see #755)
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.

4 participants