Global: Fix some DeepScan issues. #23351
Conversation
| this.workerPool.dispose(); | ||
| if ( this.workerSourceURL ) URL.revokeObjectURL( this.workerSourceURL ); |
There was a problem hiding this comment.
@donmccurdy dispose() was implemented two times. I hope this final version is the correct one 😉 .
| @@ -124,7 +124,8 @@ function isUniqueEdge( start, end, edges ) { | |||
|
|
|||
| } else { | |||
|
|
|||
| edges.add( hash1, hash2 ); | |||
| edges.add( hash1 ); | |||
| edges.add( hash2 ); | |||
There was a problem hiding this comment.
This is actually an important fix since hash2 was never added to edges.
There was a problem hiding this comment.
So it means that we were still generating duplicated lines?
There was a problem hiding this comment.
Both hashes should be part of edges. The new code ensures that this is the case and duplicates should be avoided now.
There was a problem hiding this comment.
Both hashes should be part of edges.
I do not think that is necessary.
There was a problem hiding this comment.
After a second look it's indeed sufficient to only store a single edge. The coincident edge is not required in the set since when an edge is tested, isUniqueEdge() generates two hashes for testing the actual and the coincident edge.
However, I don't think the logic is "wrong". Storing both edges allows a potential early out in isUniqueEdge() ( you can safe a call of Set.has()).
There was a problem hiding this comment.
After a second look it's indeed sufficient to only store a single edge.
As I said, the coincident edge did not have to be added to the set in this PR.
If you insist on adding both edges to the set, then you can compute hash2 only when it is added. Your choice. :-)
Yeah! |
|
Thanks! |
Related issue: -
Description
It's impressive how many issues DeepScan can detect. I've fixed some of them with this PR.