Digraphs uses NEW_PLIST_WITH_MUTABILITY which is not present in GAP 4.9.0

[james-d-mitchell]

Apparently, in v1.0.1, if I try to compile Digraphs with GAP v4.9.0 and I get the following warnings:

src/digraphs.c:88:13: warning: implicit declaration of function
      'NEW_PLIST_WITH_MUTABILITY' is invalid in C99
      [-Wimplicit-function-declaration]
      ret = NEW_PLIST_WITH_MUTABILITY(mut, T_PLIST_EMPTY, 0);
            ^
src/digraphs.c:88:11: warning: incompatible integer to pointer conversion
      assigning to 'Obj' (aka 'unsigned long long **') from 'int'
      [-Wint-conversion]
      ret = NEW_PLIST_WITH_MUTABILITY(mut, T_PLIST_EMPTY, 0);
          ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/digraphs.c:90:13: warning: implicit declaration of function
      'NEW_PLIST_WITH_MUTABILITY' is invalid in C99
      [-Wimplicit-function-declaration]
      ret = NEW_PLIST_WITH_MUTABILITY(mut, T_PLIST_TAB, n);
            ^
src/digraphs.c:90:11: warning: incompatible integer to pointer conversion
      assigning to 'Obj' (aka 'unsigned long long **') from 'int'
      [-Wint-conversion]
      ret = NEW_PLIST_WITH_MUTABILITY(mut, T_PLIST_TAB, n);
          ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/digraphs.c:126:1: warning: control may reach end of non-void function
      [-Wreturn-type]
}

And indeed NEW_PLIST_WITH_MUTABILITY does not exist in GAP v4.9.0. This should be easy to fix.

[wilfwilson]

Thanks for noticing this @james-d-mitchell. I'm curious why this isn't actually causing any of our tests to fail. (We test the required version of GAP, currently 4.9.0, in one Travis job, e.g. https://travis-ci.org/gap-packages/Digraphs/jobs/598715505).

Do you think that means we have no coverage of these lines? Or is something else going on?

[james-d-mitchell]

Yeah I noticed this too, it must mean that those lines of code aren't tested, or they are and something more complicated is going on...

[wilfwilson]

I think this code is never executed. I thought I could trigger the relevant code by creating a digraph via source and range, and then asking for the out-neighbours. When I create such a digraph, it claims not to know its out-neighbours, but in fact they are already known and stored in its OutNeighbours record component:

gap> r := rec(DigraphNrVertices := 0, DigraphRange := [], DigraphSource := []);;
gap> D := Digraph(r);;
gap> HasOutNeighbours(D);
false
gap> NamesOfComponents(D);
[ "DigraphNrEdges", "OutNeighbours" ]

This is because digraphs are ultimately (as far as I can tell) created via the following code, which directly constructs the out-neighbours from the source-and range using the separate function DIGRAPH_OUT_NEIGHBOURS_FROM_SOURCE_RANGE:

InstallMethod(DigraphConsNC, "for IsMutableDigraph and a record",
[IsMutableDigraph, IsRecord],
function(filt, record)
  [...omitted...]
  out := DIGRAPH_OUT_NEIGHBOURS_FROM_SOURCE_RANGE(record.DigraphNrVertices,
                                                  record.DigraphSource,
                                                  record.DigraphRange);
  return ConvertToMutableDigraphNC(out);
end);

I think this means that anything that is a digraph object already necessarily knows its OutNeighbours. Therefore the C function FuncOutNeighbours on digraphs.c:76 always returns on its second line, and never reaches the problematic code.

That explains why this use of NEW_PLIST_WITH_MUTABILITY is not breaking anything. So should we simply remove this code now? (And think about getting automated code coverage for our kernel code...)