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

Don't copy an immutable digraph when adding zero vertices #346

Merged
merged 1 commit into from
Nov 18, 2020
Merged

Don't copy an immutable digraph when adding zero vertices #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions doc/oper.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,10 @@ true

If <A>digraph</A> belongs to <Ref Filt="IsMutableDigraph"/>, then the
vertices are added directly to <A>digraph</A>, which is changed in-place.
If <A>digraph</A> belongs to <Ref Filt="IsImmutableDigraph"/>, then the
result is returned as an immutable digraph.
If <A>digraph</A> belongs to <Ref Filt="IsImmutableDigraph"/>, then
<A>digraph</A> itself is returned if no vertices are added
(i.e. <C><A>m</A>=0</C> or <A>labels</A> is empty), otherwise
the result is a new immutable digraph.
<P/>

<Example><![CDATA[
Expand Down
15 changes: 12 additions & 3 deletions gap/oper.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,12 @@ end);

InstallMethod(DigraphAddVertices, "for an immutable digraph and list",
[IsImmutableDigraph, IsList],
{D, labels} -> MakeImmutable(DigraphAddVertices(DigraphMutableCopy(D),
labels)));
function(D, labels)
if IsEmpty(labels) then
Copy link
Member

Choose a reason for hiding this comment

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

I know you didn't name this in your PR, but why is the argument named labels?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because this method adds Length(labels) new vertices, where the ith vertex to be added is given the label labels[i].

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks!

return D;
fi;
return MakeImmutable(DigraphAddVertices(DigraphMutableCopy(D), labels));
end);

InstallMethod(DigraphAddVertices, "for a mutable digraph and an integer",
[IsMutableDigraph, IsInt],
Expand All @@ -79,7 +83,12 @@ end);

InstallMethod(DigraphAddVertices, "for an immutable digraph and an integer",
[IsImmutableDigraph, IsInt],
{D, m} -> MakeImmutable(DigraphAddVertices(DigraphMutableCopy(D), m)));
function(D, m)
if m = 0 then
return D;
fi;
return MakeImmutable(DigraphAddVertices(DigraphMutableCopy(D), m));
end);

# Included for backwards compatibility, even though the 2nd arg is redundant.
# See https://github.com/digraphs/Digraphs/issues/264
Expand Down
12 changes: 2 additions & 10 deletions tst/standard/oper.tst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,17 +617,9 @@ gap> DigraphVertexLabels(gr2);
[ Alt( [ 1 .. 5 ] ), Sym( [ 1 .. 2 ] ), Group(()) ]
gap> DigraphAddVertices(gr2, -1);
Error, the 2nd argument <m> must be a non-negative integer,
gap> gr3 := DigraphAddVertices(gr2, 0);
<immutable digraph with 3 vertices, 1 edge>
gap> IsIdenticalObj(gr2, gr3);
false
gap> gr2 = gr3;
gap> IsIdenticalObj(gr2, DigraphAddVertices(gr2, 0));
true
gap> gr3 := DigraphAddVertices(gr2, []);
<immutable digraph with 3 vertices, 1 edge>
gap> IsIdenticalObj(gr2, gr3);
false
gap> gr2 = gr3;
gap> IsIdenticalObj(gr2, DigraphAddVertices(gr2, []));
true

# DigraphAddVertices (redundant three-argument version)
Expand Down