Introduce -N switch to not use hidden implications #2246

frankluebeck · 2018-03-06T22:36:17Z

If GAP is normally called then this patch only causes

that the kernel list HIDDEN_IMPS is visible from GAP level
that two debugging functions are available
- WriteFilterRanks(filename);
- WriteMethodOverview(filename);

Furthermore a command line switch -N is introduced. When gap is called with
this -N then:

no hidden implications are installed anymore
RankFilter only uses proper implications
InstallMethod uses only proper implication during its check if
the installation fits to one declaration. When it does not find
an appropriate declaration then it does not run into an error but only
prints a line of useful information. (This allows to systematically
either add explicit filter implications or change the filters in some
method installations until these messages disappear.)

This patch should be safe to be merged into master. Actual changes in
filter implications will be provided in separate patches.

frankluebeck · 2018-03-06T22:37:39Z

This could also be interesting in the context of pull request #2222.

fingolfin

I did not yet have a chance to look at this closer, but I really like the general direction!

fingolfin · 2018-03-06T23:24:37Z

lib/oper1.g

+ Error("the number of arguments does not match a declaration of ",
+ NAME_FUNC(opr) );
+ else
+ Print("IM: the number of arguments does not ",


Why "IM: "?

Perhaps "IM" is meant to be a shorthand for InstallMethod?

If one sees this message as a user, it's a bit unclear what it implies. Perhaps this would be better? "Warning, the number of ..."

ChrisJefferson · 2018-03-24T10:46:30Z

So I had a look at this, it looks fine, the only problem is the same changes need making to HPC-GAP, else (at the moment) this breaks HPC-GAP from starting.

fingolfin · 2018-03-27T13:49:31Z

I am also somewhat unhappy about using up a single-letter option for this rather obscure debugging tool. How about instead naming it, --disable-hidden-implications or --no-hidden-imps or so?

frankluebeck · 2018-05-28T15:34:28Z

@ChrisJefferson: I think it is just not needed that this (hopefully short term) temporary switch works with HPCGAP.

@fingolfin: I chose -N because it was already there (but not used for anything). Since the switch is really only meant for temporary debugging I would prefer to not put more effort in introducing a new switch in all necessary places.

fingolfin · 2018-05-29T09:49:41Z

lib/oper1.g

+ Error("the number of arguments does not match a declaration of ",
+ NAME_FUNC(opr) );
+ else
+ Print("IM: the number of arguments does not ",


Perhaps "IM" is meant to be a shorthand for InstallMethod?

If one sees this message as a user, it's a bit unclear what it implies. Perhaps this would be better? "Warning, the number of ..."

fingolfin · 2018-05-29T09:54:53Z

lib/operdebug.g

+ nam := NameFunction(op);
+ for i in [0..6] do
+ m := METHODS_OPERATION(op, i);
+ s := 4+i;


This needs to be changed to s := BASE_SIZE_METHODS_OPER_ENTRY + i;

I also suggest rebasing this PR on master and verifying these functions still work. Ideally, there'd be a test case for them, too, just to make sure we don't break them horribly. Of course one may argue they are just debugging tools; but also debugging tools are more useful if in order to use them you don't first have to track down what caused a regression in them some time in the past ;-).

fingolfin · 2018-05-29T09:55:21Z

lib/operdebug.g

+ for j in [1..Length(m)/s] do
+ Print(nam,";\c",i,";",
+ name(m[j*s-2]),
+ sline(m[j*s-2]),";\c",


This writes out the location of the definition of the function, if available. It does not yet take advantage of the location of the method installation, which we now also track (see e.g. the code of ApplicableMethodTypes)

fingolfin · 2018-05-29T09:57:25Z

src/opers.c

@@ -4394,6 +4394,7 @@ static Int InitLibrary (
 SET_LEN_PLIST(HIDDEN_IMPS, 0);
 WITH_HIDDEN_IMPS_FLAGS_CACHE = NEW_PLIST(T_PLIST, HIDDEN_IMPS_CACHE_LENGTH * 2);
 SET_LEN_PLIST(WITH_HIDDEN_IMPS_FLAGS_CACHE, HIDDEN_IMPS_CACHE_LENGTH * 2);
+ AssGVar(GVarName("HIDDEN_IMPS"), HIDDEN_IMPS);


Why is this needed?

To be able to see the content of HIDDEN_IMPS from GAP level.

fingolfin · 2018-05-29T09:58:49Z

lib/filter.g

- InstallHiddenTrueMethod( tofilt, from );
+ if not GAPInfo.CommandLineOptions.N then
+ InstallHiddenTrueMethod( tofilt, from );
+ fi;


Wouldn't it be much easier to put the check for the command line option into InstallHiddenTrueMethod? Then one can even keep calling WITH_HIDDEN_IMPS_FLAGS below, as we'd be guaranteed that there are not hidden implications beyond the regular implications, and almost all changes on the library side could be removed.

The if GAPInfo.CommandLineOptions.N are introduced such that it is easy to find the places where the behaviour is changed by the -N switch.

frankluebeck · 2018-05-29T16:02:06Z

The branch is

rebased on current master
the "IM" in messages is changed to "InstallMethod warning"
the utility debug function WriteMethodOverview is adjusted to use the new info on method installation locations
added one line comments before the two utility functions
also adjusted the hpcgap/src/c_*.c files

codecov · 2018-05-29T16:04:27Z

Codecov Report

Merging #2246 into master will decrease coverage by 0.05%.
The diff coverage is 42.39%.

@@            Coverage Diff             @@
##           master    #2246      +/-   ##
==========================================
- Coverage   74.27%   74.21%   -0.06%     
==========================================
  Files         484      485       +1     
  Lines      245343   245597     +254     
==========================================
+ Hits       182225   182280      +55     
- Misses      63118    63317     +199

Impacted Files	Coverage Δ
lib/system.g	`67.13% <ø> (ø)`	⬆️
lib/operdebug.g	`0% <0%> (ø)`
src/opers.c	`94.01% <100%> (ø)`	⬆️
lib/oper.g	`80.56% <100%> (+0.27%)`	⬆️
src/c_oper1.c	`88.03% <42.3%> (-2.85%)`	⬇️
hpcgap/src/c_oper1.c	`85.83% <45.51%> (-2.91%)`	⬇️
lib/filter.g	`94.01% <87.5%> (-0.58%)`	⬇️
hpcgap/lib/hpc/stdtasks.g	`63.42% <0%> (-1.28%)`	⬇️
src/compiler.c	`87.25% <0%> (+0.04%)`	⬆️
... and 3 more

If GAP is normally called then this patch only causes - that the kernel list HIDDEN_IMPS is visible from GAP level - that two debugging functions are available - WriteFilterRanks(filename); - WriteMethodOverview(filename); Furthermore a command line switch -N is introduced. When gap is called with this -N then: - no hidden implications are installed anymore - RankFilter only uses proper implications - InstallMethod uses only proper implication during its check if the installation fits to one declaration. When it does not find an appropriate declaration then it does not run into an error but only prints a line of useful information. (This allows to systematically either add explicit filter implications or change the filters in some method installations until these messages disappear.) This patch should be safe to be merged into master. Actual changes in filter implications will be provided in separate patches.

olexandr-konovalov · 2018-09-02T16:40:00Z

@frankluebeck any reasons why this is not documented in Chapter 3 ?

Perhaps one should check whether there are more cases like this...

olexandr-konovalov · 2018-09-02T16:42:34Z

lib/system.g

@@ -92,7 +92,7 @@ BIND_GLOBAL( "GAPInfo", rec(
 rec( short:= "B", default := "", arg := "<name>", help := [ "current architecture"] ),
 rec( short:= "D", default := false, help := ["enable/disable debugging the loading of files"] ),
 rec( short:= "M", default := false, help := ["disable/enable loading of compiled modules"] ),
- rec( short:= "N", default := false, help := ["unused, for backward compatibility only"] ),
+ rec( short:= "N", default := false, help := ["do not use hidden implications"] ),


@frankluebeck The comment above says "These options must be kept in sync with those in system.c" but that file is not even modified by this PR. Am I missing something?

frankluebeck · 2018-09-03T14:34:51Z

@alex-konovalov: As discussed above, this -N switch is a temporary change, just for debugging. This should not be documented. Once the "hidden implications" are no longer used we can revert the change in 'system.g' (and simplify the few places where the -N switch is used).

frankluebeck mentioned this pull request Mar 6, 2018

Add explicit implications #2247

Closed

fingolfin reviewed Mar 6, 2018

View reviewed changes

This was referenced Mar 30, 2018

Plan: get rid of hidden implications #2336

Open

Turn hidden implications into actual implications #2222

Merged

fingolfin reviewed May 29, 2018

View reviewed changes

frankluebeck force-pushed the fl_nohiddenimps branch from f7d12b1 to 3bf1d53 Compare May 29, 2018 15:51

frankluebeck force-pushed the fl_nohiddenimps branch from 3bf1d53 to 1b35735 Compare May 30, 2018 08:07

frankluebeck force-pushed the fl_nohiddenimps branch from 1b35735 to 287cb26 Compare May 30, 2018 09:02

ChrisJefferson approved these changes Jun 2, 2018

View reviewed changes

fingolfin approved these changes Jun 5, 2018

View reviewed changes

fingolfin merged commit 954b478 into gap-system:master Jun 5, 2018

olexandr-konovalov reviewed Sep 2, 2018

View reviewed changes

fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Sep 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce -N switch to not use hidden implications #2246

Introduce -N switch to not use hidden implications #2246

frankluebeck commented Mar 6, 2018

frankluebeck commented Mar 6, 2018

fingolfin left a comment

fingolfin Mar 6, 2018

fingolfin May 29, 2018

ChrisJefferson commented Mar 24, 2018

fingolfin commented Mar 27, 2018

frankluebeck commented May 28, 2018

fingolfin May 29, 2018

fingolfin May 29, 2018

fingolfin May 29, 2018

fingolfin May 29, 2018

fingolfin May 29, 2018

frankluebeck May 29, 2018

fingolfin May 29, 2018

frankluebeck May 29, 2018

frankluebeck commented May 29, 2018 •

edited

Loading

codecov bot commented May 29, 2018 •

edited

Loading

olexandr-konovalov commented Sep 2, 2018

olexandr-konovalov Sep 2, 2018

frankluebeck commented Sep 3, 2018

Introduce -N switch to not use hidden implications #2246

Introduce -N switch to not use hidden implications #2246

Conversation

frankluebeck commented Mar 6, 2018

frankluebeck commented Mar 6, 2018

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

ChrisJefferson commented Mar 24, 2018

fingolfin commented Mar 27, 2018

frankluebeck commented May 28, 2018

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

frankluebeck commented May 29, 2018 • edited Loading

codecov bot commented May 29, 2018 • edited Loading

Codecov Report

olexandr-konovalov commented Sep 2, 2018

Choose a reason for hiding this comment

frankluebeck commented Sep 3, 2018

frankluebeck commented May 29, 2018 •

edited

Loading

codecov bot commented May 29, 2018 •

edited

Loading