Give human-readable names to functions in InstallMethod

[ChrisJefferson]

@markusbaumeister pointed out that many library functions show up as "nameless" in profiling output. This turned out to be because InstallMethod does not give the functions a useful name. This is a shame, as it's easy to give them a very informative name in InstallMethod. This PR does that.

This shouldn't cause any problems, apart from using a small amount of extra memory on strings. I decided to also give a useful name to the functions which calculate priority. While these functions are very small, it's still useful to know what they are in a trace.

Text for release notes

• Give more library methods human-readable names. These are used when profiling.

[codecov]

Codecov Report

Merging #3420 into master will increase coverage by <.01%.
The diff coverage is 91.23%.

@@            Coverage Diff             @@
##           master    #3420      +/-   ##
==========================================
+ Coverage   85.34%   85.35%   +<.01%     
==========================================
  Files         699      699              
  Lines      346091   346427     +336     
==========================================
+ Hits       295373   295676     +303     
- Misses      50718    50751      +33

Impacted Files	Coverage Δ
src/c_oper1.c	86.26% <91.23%> (+0.2%)	⬆️
src/hpc/c_oper1.c	85.9% <91.23%> (+0.21%)	⬆️
src/objset.c	85.1% <0%> (+0.22%)	⬆️

[coveralls]

Coverage increased (+0.006%) to 85.16% when pulling 671e5d1 on ChrisJefferson:name_func into 578ea99 on gap-system:master.

[fingolfin]

@ChrisJefferson please assign labels to your PRs.

[fingolfin]

Looks sensible to me overall, but a comment needs to be adjusted. Also, some musings...

[olexandr-konovalov]

@ChrisJefferson I find the description for release notes "Give more library functions descriptive names when profiling" to be misleading. This PR does not seem to change any names - it intends to add ability to do this, and apparently adds new functions to do this, which should be then mentioned in release notes (and linked in their GAPDoc version to appropriate mansections).

[ChrisJefferson]

Improved code (although I haven't switched to an attribute). Linked up docs, and also linked up the unused doc for SetNameFunction.

I put two things in the release notes, as I feel the better names are important (and this PR does add better names, that's the point). Adding "HasNameFunction" doesn't feel like something most people would care about, so it should probably be mentioned in some kind of "minor" section (or it can be put with all the other release notes, don't mind).

[ChrisJefferson]

I'm happy to rebase this, is this PR fine as it is? (I don't mind if someone else wants to add #3430 on top later).

[fingolfin]

@ChrisJefferson as @alex-konovalov noted, the title of this PR is misleading, and unsuitable for release notes.

[fingolfin]

OK, so this now obviously needs to be rebased... sorry for the hassle.

[ChrisJefferson]

I can't fix this, as I can't call HasNameFunction in oper.g, and I can't figure out where it is declared to know what/if it's "internal" name is.

[fingolfin]

@ChrisJefferson TESTER_FILTER(NAME_FUNC)

[ChrisJefferson]

Nope, segfaults

[fingolfin]

Ahh, of course: As long as HasNameFunction is not yet defined, also the type objects of functions ( TYPE_FUNCTION, TYPE_FUNCTION_WITH_NAME, TYPE_OPERATION and TYPE_OPERATION_WITH_NAME) are not yet defined. But to test the filter, we need the type object. Hence this can't work.

[fingolfin]

So one quick fix would be to check for IsBound(TYPE_FUNCTION) or IsBound(HasNameFunction), and only call the tests in that case. This way, only very early method installations won't be covered (we are talking about a single digit number of them), but everything else later one should work.

[ChrisJefferson]

Nope, still segfaults (see my pushed version, I check for IsBound(HasNameFunction) and IsBound(TYPE_FUNCTION) and IsBound(TYPE_FUNCTION_WITH_NAME) and IsBound(TYPE_OPERATION_WITH_NAME) and VAL_GVAR("HasNameFunction")(method))

[fingolfin]

Interesting. I can't reproduce a segfault here with your branch. I do get a strange error, though:

$ ./gap -A
#W Static module lib/oper1.g has CRC mismatch, ignoring
 ┌───────┐   GAP 4.10dev-2036-gae0e26a of today
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-apple-darwin18.5.0-default64-kv6
 Configuration:  gmp 6.1.2, GASMAN, readline
 Loading the library and packages ...
Error, Record Element: '<rec>.GeneratorsOfMagmaWithInverses' must have an assigned value in
  GeneratorsOfGroup( F ) at /Users/mhorn/Projekte/GAP/gap.github/lib/grpfp.gi:1583 called from
<function "/ for a free group and an empty list of relators">( <arguments> )
 called from read-eval loop at /Users/mhorn/Projekte/GAP/gap.github/lib/grpfp.gi:5666
type 'quit;' to quit to outer loop
brk>

[fingolfin]

I guess the cause for the error is that w/o your patch, I get this:

gap> NameFunction(GeneratorsOfMagmaWithInverses);
"GeneratorsOfMagmaWithInverses"

but with it, I get this, which then breaks stuff:

brk> NameFunction(GeneratorsOfMagmaWithInverses);
"FreeGeneratorsOfFpGroup for a free group"

I don't have time to track it down further right now, sorry.

[ChrisJefferson]

Hopefully now all updated + cleaned up.

[fingolfin]

So, I am in principle fine with keeping this as is, but here's an alternative that might perhaps be a bit easier to read, and which would ensure that the handful of methods which are installed before HasNameFunction is defined properly get names, too: Right before this function, insert HasNameFunction := ReturnFalse;, possibly with a comment explaining what's going on. Then Unbind(HasNameFunction); right before HasNameFunction is actually defined. And here, we can then simply return to the original form, i.e.:

     if IS_FUNCTION(method) and HasNameFunction(method) then

and of course similar for the rank function

[fingolfin]

To be clear, I am OK with this is merged as-is, though you may wish to at least ponder my suggestion :-).