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.

Sign up for GitHub

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

Jump to bottom

Idea: record location of InstallMethod call #2239

Closed
fingolfin opened this issue Mar 4, 2018 · 2 comments · Fixed by #2393
Closed

Idea: record location of InstallMethod call #2239

fingolfin opened this issue Mar 4, 2018 · 2 comments · Fixed by #2393
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements

Comments

@fingolfin
Copy link
Member

Right now, one can use e.g. LocationFunc or PageSource on methods installed for an operation, which is quite handy. But sometimes the same function is installed several times; or a kernel function is installed as a method (possibly also several times), and in these cases, the location of the function definition does not coincide with the location of the method installation.

Thus, it would be nice if we could store the location of the method installation, too. I assume it could be e.g. stored in the methods array, along with the other methods.

The tricky part might be to figure out the location itself. I assume this may require some work on the kernel to be possible. We'd somehow need to retrieve the location of the place InstallMethod was called... Perhaps some clever lvar walking can help?!? hum
@markuspf
Copy link
Member

markuspf commented Mar 4, 2018

I have some prototype code that does this (in multiple variations).

My first incarnation had a global variable that complemented the OPERATIONS array.

I think an approach similar to GET_OPER_FLAGS (which stores the location of the declarations for each operation already) can work here, too.

@fingolfin fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Mar 9, 2018
@fingolfin
Copy link
Member Author

Well, I assume we'd have to add one more entry per method into each METHODS_OPERATION(oper,n) list, storing the location info there. Of course that in turn requires adjusting all places that deal with METHODS_OPERATION -- so at least INSTALL_METHOD_FLAGS and the kernel code in GetMethodUncached

fingolfin added a commit to fingolfin/gap that referenced this issue Apr 22, 2018
@fingolfin
When tracing immediates, print location of definition
45d0448 
For now, we print the location of the *method*. Ideally, we should
instead track the location of the InstallImmdiateMethod call. See also
issue gap-system#2239.
fingolfin added a commit to fingolfin/gap that referenced this issue Apr 22, 2018
@fingolfin
When tracing immediates, print location of definition
e87056b 
For now, we print the location of the *method*. Ideally, we should
instead track the location of the InstallImmdiateMethod call. See also
issue gap-system#2239.

Before:

gap> TraceImmediateMethods( );
gap> g:= Group( (1,2,3), (1,2) );;

After:

gap> TraceImmediateMethods( );
gap> g:= Group( (1,2,3), (1,2) );;
@fingolfin fingolfin mentioned this issue Apr 22, 2018
Closed
fingolfin added a commit to fingolfin/gap that referenced this issue Apr 23, 2018
@fingolfin
When tracing immediates, print location of definition
3428047 
For now, we print the location of the *method*. Ideally, we should
instead track the location of the InstallImmdiateMethod call. See also
issue gap-system#2239.

Before:

    gap> TraceImmediateMethods( );
    gap> g:= Group( (1,2,3), (1,2) );;
    #I  immediate: Size
    #I  immediate: IsCyclic
    #I  immediate: IsCommutative
    #I  immediate: IsTrivial

After:

    gap> TraceImmediateMethods( );
    gap> g:= Group( (1,2,3), (1,2) );;
    #I RunImmediateMethods
    #I  immediate: Size at GAPROOT/lib/coll.gi:174
    #I  immediate: IsCyclic at GAPROOT/lib/grp.gi:34
    #I  immediate: IsCommutative at GAPROOT/lib/magma.gi:190
    #I  immediate: IsTrivial at GAPROOT/lib/magma.gi:124
fingolfin added a commit to fingolfin/gap that referenced this issue Apr 23, 2018
@fingolfin
When tracing immediates, print location of definition
6d3fff5 
For now, we print the location of the *method*. Ideally, we should
instead track the location of the InstallImmediateMethod call. See also
issue gap-system#2239.

Before:

    gap> TraceImmediateMethods( );
    gap> g:= Group( (1,2,3), (1,2) );;
    #I  immediate: Size
    #I  immediate: IsCyclic
    #I  immediate: IsCommutative
    #I  immediate: IsTrivial

After:

    gap> TraceImmediateMethods( );
    gap> g:= Group( (1,2,3), (1,2) );;
    #I RunImmediateMethods
    #I  immediate: Size at GAPROOT/lib/coll.gi:174
    #I  immediate: IsCyclic at GAPROOT/lib/grp.gi:34
    #I  immediate: IsCommutative at GAPROOT/lib/magma.gi:190
    #I  immediate: IsTrivial at GAPROOT/lib/magma.gi:124
fingolfin added a commit to fingolfin/gap that referenced this issue Apr 23, 2018
@fingolfin
When tracing immediates, print location of definition
6c5c1a0 
For now, we print the location of the *method*. Ideally, we should
instead track the location of the InstallImmediateMethod call. See also
issue gap-system#2239.

Before:

    gap> TraceImmediateMethods( );
    gap> g:= Group( (1,2,3), (1,2) );;
    #I  immediate: Size
    #I  immediate: IsCyclic
    #I  immediate: IsCommutative
    #I  immediate: IsTrivial

After:

    gap> TraceImmediateMethods( );
    gap> g:= Group( (1,2,3), (1,2) );;
    #I RunImmediateMethods
    #I  immediate: Size at GAPROOT/lib/coll.gi:174
    #I  immediate: IsCyclic at GAPROOT/lib/grp.gi:34
    #I  immediate: IsCommutative at GAPROOT/lib/magma.gi:190
    #I  immediate: IsTrivial at GAPROOT/lib/magma.gi:124
fingolfin added a commit to fingolfin/gap that referenced this issue Apr 23, 2018
@fingolfin
When tracing immediates, print location of definition
5e09309 
For now, we print the location of the *method*. Ideally, we should
instead track the location of the InstallImmediateMethod call. See also
issue gap-system#2239.

Before:

    gap> TraceImmediateMethods( );
    gap> g:= Group( (1,2,3), (1,2) );;
    #I  immediate: Size
    #I  immediate: IsCyclic
    #I  immediate: IsCommutative
    #I  immediate: IsTrivial

After:

    gap> TraceImmediateMethods( );
    gap> g:= Group( (1,2,3), (1,2) );;
    #I RunImmediateMethods
    #I  immediate: Size at GAPROOT/lib/coll.gi:174
    #I  immediate: IsCyclic at GAPROOT/lib/grp.gi:34
    #I  immediate: IsCommutative at GAPROOT/lib/magma.gi:190
    #I  immediate: IsTrivial at GAPROOT/lib/magma.gi:124
fingolfin added a commit to fingolfin/gap that referenced this issue Apr 23, 2018
@fingolfin
Track location of InstallMethod
337faf5 
This makes the information displayed by TraceMethods and by
ApplicableMethod much more useful in many case, which resolves gap-system#2239.
For example, consider this method for `AsList`, installed in coll.gi:

    InstallMethod( AsList,
        "for collections that are constant time access lists",
        [ IsCollection and IsConstantTimeAccessList ],
        Immutable );

With GAP 4.8, no location information is shown at all:

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists

With GAP 4.9 / master, the location where the method (i.e., the
underlying function) was defined is shown, which already does the job in
many cases, but not here:

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists at src/objects.c:fail

With this PR, we get this (note that instead of GAPROOT, the actual path
is printed)

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists at GAPROOT/lib/coll.gi:302
@fingolfin fingolfin mentioned this issue Apr 23, 2018
Merged
fingolfin added a commit to fingolfin/gap that referenced this issue Apr 23, 2018
@fingolfin
Track location of InstallMethod
bc2c12a 
This makes the information displayed by TraceMethods and by
ApplicableMethod much more useful in many case, which resolves gap-system#2239.
For example, consider this method for `AsList`, installed in coll.gi:

    InstallMethod( AsList,
        "for collections that are constant time access lists",
        [ IsCollection and IsConstantTimeAccessList ],
        Immutable );

With GAP 4.8, no location information is shown at all:

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists

With GAP 4.9 / master, the location where the method (i.e., the
underlying function) was defined is shown, which already does the job in
many cases, but not here:

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists at src/objects.c:fail

With this PR, we get this (note that instead of GAPROOT, the actual path
is printed)

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists at GAPROOT/lib/coll.gi:302
fingolfin added a commit that referenced this issue Apr 23, 2018
@fingolfin
Track location of InstallMethod
a68db9d 
This makes the information displayed by TraceMethods and by
ApplicableMethod much more useful in many case, which resolves #2239.
For example, consider this method for `AsList`, installed in coll.gi:

    InstallMethod( AsList,
        "for collections that are constant time access lists",
        [ IsCollection and IsConstantTimeAccessList ],
        Immutable );

With GAP 4.8, no location information is shown at all:

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists

With GAP 4.9 / master, the location where the method (i.e., the
underlying function) was defined is shown, which already does the job in
many cases, but not here:

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists at src/objects.c:fail

With this PR, we get this (note that instead of GAPROOT, the actual path
is printed)

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists at GAPROOT/lib/coll.gi:302
ChrisJefferson pushed a commit to ChrisJefferson/gap that referenced this issue Jun 14, 2018
@fingolfin @ChrisJefferson
Track location of InstallMethod
19dbfdc 
This makes the information displayed by TraceMethods and by
ApplicableMethod much more useful in many case, which resolves gap-system#2239.
For example, consider this method for `AsList`, installed in coll.gi:

    InstallMethod( AsList,
        "for collections that are constant time access lists",
        [ IsCollection and IsConstantTimeAccessList ],
        Immutable );

With GAP 4.8, no location information is shown at all:

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists

With GAP 4.9 / master, the location where the method (i.e., the
underlying function) was defined is shown, which already does the job in
many cases, but not here:

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists at src/objects.c:fail

With this PR, we get this (note that instead of GAPROOT, the actual path
is printed)

    gap> TraceMethods(AsList); AsList("abc");;
    #I  AsList: for collections that are constant time access lists at GAPROOT/lib/coll.gi:302
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Track location of InstallMethod and InstallImmediateMethod fingolfin/gap
2 participants
@fingolfin @markuspf