KeRNeLith / QuikGraph

Generic Graph Data Structures and Algorithms for .NET
https://kernelith.github.io/QuikGraph/
Microsoft Public License
453 stars 65 forks source link

[BUG] Seems like invalid usage of XmlReader in DeserializeFromXmlInternal method #35

Closed Vasar007 closed 2 years ago

Vasar007 commented 2 years ago

Describe the bug I have read source code recently. Here I noticed that edgePredicate is invoked with parent reader, not subtree reader.

if (vertexPredicate(xmlReader))
{
    TVertex vertex = vertexFactory(xmlReader);
    val.AddVertex(vertex);
}
else if (edgePredicate(reader)) // Why do we prefer "reader" over "xmlReader" here?
{
    TEdge edge = edgeFactory(xmlReader);
    val.AddEdge(edge);
}

Expected behavior I think subtree reader should be used in the code above like for vertexPredicate. If current logic is valid, I suggest to write comment that such code is not copypaste error.

Additional context None

KeRNeLith commented 2 years ago

It seems the result of both is the same since the reader stream is certainly at the same place, but you're right there is no reason to not use the subtree reader like for vertices, it will be clearer. This seems to be a really old naming issue I didn't see during library cleaning.

I committed a fix (f690bd1dcdba7a06cb29709a7d638d6cbff03f19) for this.

Thank you for this good catch ;-)