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.

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

Value types break EdmondsKarpMaximumFlowAlgorithm (and potentially others) #186

Open
ktnr opened this issue Nov 12, 2018 · 4 comments
Open

Comments

@ktnr
Copy link
Contributor

ktnr commented Nov 12, 2018

EdmondsKarpMaximumFlowAlgorithm works fine if TEdge is a reference type, e.g. Edge or TaggedEdge. If, however, TEdge is a value type (e.g. STaggedEdge or STaggedEquatableEdge) the algorithm fails:

In ReversedEdgeAugmentorAlgorithm<TVertex, TEdge> the method

private TEdge FindReversedEdge(TEdge edge)
{
    foreach (var redge in this.VisitedGraph.OutEdges(edge.Target))
        if (redge.Target.Equals(edge.Source))
            return redge;
    return default(TEdge);
}

returns default(TEdge). If TEdge is a reference type default(TEdge) returns null... So, the check on line 97 in ReversedEdgeAugmentorAlgorithm<TVertex, TEdge>.AddReversedEdges()
if (reversedEdge != null)
is valid. If, however, TEdge is a value type, then default(TEdge) returns null->null and the check on line 97 becomes invalid. The next check on line 102
if (!this.reversedEdges.ContainsKey(reversedEdge))
will then throw a System.NullReferenceException.

This kind of null check can be found in other parts of QuickGraph, too.

@ktnr
Copy link
Contributor Author

ktnr commented Aug 2, 2019

To properly check against reference and value types we could replace
if (reversedEdge != null)
with
if (EqualityComparer<TEdge>.Default.Equals(reversedEdge, default(TEdge)))

For reference, see https://stackoverflow.com/a/864860/5587903

This would require a lot of changes throughout the code base. Not only on null checks for TEdge but also for TVertex.
I am currently not working to fix this.

@ktnr ktnr closed this as completed Aug 2, 2019
@ktnr
Copy link
Contributor Author

ktnr commented Aug 2, 2019

Accidentaly closed the issue. It should still be open.

@ktnr ktnr reopened this Aug 2, 2019
@Orace
Copy link
Contributor

Orace commented Nov 12, 2019

The C# language since v7.1 allow to write if (reversedEdge != default)

Anyway you have to be sure that the default value will not be meaningful in some corner case.

@KeRNeLith
Copy link

I think the best way to handle such thing is to use some TryXXX pattern. Because default can in some cases be a value part of the graph which would be a meaningful value.

I fixed this using this implementation.

I also checked for possible other similar problems in the library. All should be fixed in my fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants