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

Path.js code comment #22

Closed
Glidias opened this issue Sep 5, 2016 · 8 comments
Closed

Path.js code comment #22

Glidias opened this issue Sep 5, 2016 · 8 comments

Comments

@Glidias
Copy link

Glidias commented Sep 5, 2016

return otherPath.components == this.components; //originally uses SequenceEqual, not sure this achieves the same

https://msdn.microsoft.com/en-us/library/bb348567(v=vs.110).aspx

I don't think it does. You'd need to enumerate through both and compare each element within the components list and ensure the entire sequence matches. I safely assume the "default equality operator" should be referential equality (==) with the Component objects, and not necessarily the IEquatable interface implementation Equals(other:T) (that's used for something else..i think), but you might want to check with the actual devs.

Anyway, doc reference for IEquatable .
https://msdn.microsoft.com/en-us/library/ms131187(v=vs.110).aspx
I think this is mainly used for ObjectMaps like Dictionaries and such with regards to Key identity...but I can't find any reference of it being used explicitly for the IEnumerable implementations like List. Which was why I safely assumes that simple "==" should be fine... since I don't see any call made to SequenceEquals() with the optional parameter IEqualityComparer included in. Anyway, I myself just want to to be sure, because some of the "==" comparisons being made within the C# code lines might need to explicitly call the "Equals" method directly for other platforms, but I might be wrong.. One important thing is to not gloss over the operators functions in some classes, as you'd likely have to call these functions instead of using the standard code operators. (however, the operator implementations they used with regards to the Object class appear to be fairly trivial and the ecmascript equilavents should be fine..). In Haxe, anyway, there is no implicit boolean conversions, so some of the C# syntantic sugar they adopted with operator methods to circumvent the need to explicitly declare operator comparisons when checking for Runtime.Objects, doesn't apply for me. Also, with the explicit !=, == operators, I find it strange they represent != operator as !(a == b) instead of (a!=b), which makes me wonder...is there a difference? If there is, I might have to run a check and use the function definition instead, but as of now, i think typical equality operators should do fine since those methods aren't doing anything really fancy,

Anyway, just commenting/cross-referencing against yours as I'm doing up a Haxe version of this.

@y-lohse
Copy link
Owner

y-lohse commented Sep 5, 2016

Right, it most likely doesn't, except when the two pathes are literally the same, but that's a bit limited. Thanks for reminding me.

Though the question whether it uses == or the IEquatable interface is a good one. @joethephish do you mind sharing some C# insight on this?

@joethephish
Copy link
Contributor

I found the C# docs super confusing on this issue....!

When searching the current version of the codebase, that comment seems to no longer exist in my C# implementation, I seem to have swapped it back to use SequenceEqual, with the assumption being that since Path.Component implements IEquatable, it'll "do the right thing".

Given that we don't seem to have had any related bugs, I'm assuming it is doing the right thing! (yeah, I'm a professional developer, honest)

So yeah, if the JS implementation uses ==, it'll need to iterate and do a proper equality comparison.

@Glidias
Copy link
Author

Glidias commented Sep 7, 2016

so, should the IEquatable's Equals() impl be used instead (if available) when checking for equality for JS/other targets during SequenceEqual(), or would plain "==" always suffice?

@y-lohse
Copy link
Owner

y-lohse commented Sep 7, 2016

When searching the current version of the codebase, that comment seems to no longer exist in my C# implementation,

I'm pretty sure I wrote that comment as a note to myself and forgot about it later, so it never was in the C# version.

so, should the IEquatable's Equals() impl be used instead (if available) when checking for equality for JS/other targets during SequenceEqual(), or would plain "==" always suffice?

My guess is that it should use the Equals() (which is already implemented anyway). Two pathes should be considered the same if all their components are equal, not only if they are the same instance? And given that object comparisons in javascript aren't the most reliable anyway, it's probably safer.

@Glidias
Copy link
Author

Glidias commented Sep 7, 2016

Hmm..yea, there are only 2 classes (Path and Component, which implements IEquatable Equals(other:T):Bool , overriding the default Equals<Runtime.Object> implementation used by the Runtime.Object class it extends from. Looking at the code, as you said, the override is probably there for a purpose, and so I've modified my array sequence equality method with the IEquatable type constraint as well.)

public static function arraySequenceEquals<T:IEquatable<T>>(arr1:Array<T>, arr2:Array<T>):Bool {
        if (arr1.length != arr2.length) return false;
        for (i in 0...arr1.length) {
            if (!arr1[i].Equals(arr2[i])) {  // enforced IEquatable constraint
                return false;
            }
        }
        return true;
    }    

For anything outside of that SequenceEquals() case, i can't remember exactly whether there were other cases in the code where Path and Component was checked for "==" and "!=" respectively, though. May probably do a (manual) audit search for such operators being made between/ Path to Path /and /Component to Component/ just in case.

@y-lohse
Copy link
Owner

y-lohse commented Sep 7, 2016

Alright, I'll do something similar, though I'll probably inline the function. Can you let me know if you find other instances of == and != in your audit?

@Glidias
Copy link
Author

Glidias commented Sep 9, 2016

Haven't really looked into yet, but I know for one case... the dev-only debugging method BuildStringOfHierarchy might need to consider it in a sitauation where Path==Path/Component==Component comparisons are made( again...i'm only guessing), and it's more on a yagni-atm basis , since the method isn't actually used in the actual runtime for end-user.

y-lohse added a commit that referenced this issue Sep 9, 2016
@y-lohse
Copy link
Owner

y-lohse commented Sep 18, 2016

Fixed in the next release.

@y-lohse y-lohse closed this as completed Sep 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants