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

Comparison of methods in CodeHolder is wrong #71

Open
LinqLover opened this issue Jan 5, 2022 · 9 comments
Open

Comparison of methods in CodeHolder is wrong #71

LinqLover opened this issue Jan 5, 2022 · 9 comments

Comments

@LinqLover
Copy link
Collaborator

sandblocks-bug-method-equality

Please let me know if this is caused by a bug in the Trunk.

@tom95
Copy link
Collaborator

tom95 commented Jan 5, 2022

Yeah, there was a discussion about this at some point with no clear agreement in the end -- I tried finding the thread again just now but without success. Keywords should have been something along the lines of "compiledmethod" and "literals". The problem as far as I remember was that in = compiledmethod did not check the selector for equality. Or, alternatively, that the selector was not part of the method's embedded literals.

@LinqLover
Copy link
Collaborator Author

That's likely, in this case Sandblocks needs to check the selector separately, I think.

@tom95
Copy link
Collaborator

tom95 commented Jan 5, 2022

I believe it was actually a regression from behavior in 5.3, but I'll have to look it up again :/

@tom95
Copy link
Collaborator

tom95 commented Jan 5, 2022

no, not a regression. I just checked 5.3 and 5.0 and the behavior was the same. So I guess we have to contend with that behavior.

This is tricky because the methods are just another type of artefact in Sandblocks, and Sandblocks expects to be able to find that the same artefact is opened somewhere using #=, which also works for every other type of artefact that we've been using thus far. Options include littering the code with special cases for CompiledMethods, switching to identity comparisons, or adding a new sameArtefact: hook in Object.

@LinqLover
Copy link
Collaborator Author

Do you have the thread? Maybe we could change this in the Trunk ...

@tom95
Copy link
Collaborator

tom95 commented Jan 5, 2022

Thinking about it again, it probably makes sense to introduce a dedicated hook for this. In fact, comparing the selector doesn't suffice in the general case, we would also have to compare the class.

@LinqLover
Copy link
Collaborator Author

Or just switch to identity comparison. If a method has been recompiled, it should be re-rendered in Sandblocks anyway, shouldn't it?

@tom95
Copy link
Collaborator

tom95 commented Jan 5, 2022

It used to be identity comparison, which I switched when I started work on the debugger. Maybe that was a bad call. I'll try it with identity comparison for a day and see where things break :)

@LinqLover
Copy link
Collaborator Author

FYIO, CompiledMethod >> #= indeed behaves as intended in the Trunk: http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-January/218148.html

By the way, another comparison strategy would be to compare the compiled method's code references, which would also treat different versions of a method as equal (not sure if this would be intended though). :)

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

2 participants