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

Reimplement some explicitly required methods to speed up shapes #53

Closed
wants to merge 72 commits into from

Conversation

jecisc
Copy link
Member

@jecisc jecisc commented Jan 11, 2024

#explicitRequirement is really slow in the case were a method is implemented already in the superclass and this slow down a lot some visualizations. I propose to explicitly define the super call to get a huge speed up on shapes (This allows to save a lot of times in the Moose visualizations)

@Ducasse
Copy link
Contributor

Ducasse commented Jan 11, 2024

Can we remove explicit requirement instead?
Adding such code are bad.

@jecisc
Copy link
Member Author

jecisc commented Jan 11, 2024

Personally I like to have those methods because it means "Those methods needs to be there for the trait to work" like we have self subclassResponsibility to say "I'm an abstract class and this method is needed". And that helps a lot when we use a Trait we do not know.

But if asked to, I can remove them.

@Ducasse
Copy link
Contributor

Ducasse commented Jan 11, 2024

I know you like them but they do not fit with our world.
I would like to have in the signature of method the exception that they raised but this is not like that in Pharo

@guillep
Copy link
Contributor

guillep commented Jan 11, 2024

If traits were more intelligent and not install methods defined in superclasses, would that solve your problem? This of course means that adding a method in a class should recompute the "subclass responsibility/requirements" in subclasses.

Alternatively how would you feel if the classes importing a trait do never import explicit requirement methods? This means, the explicit requirement is in the trait to express the requirement, but it's not installed/compiled/copied into the class, thus producing a DNU at runtime (which is not different from the requirement error).

@jecisc
Copy link
Member Author

jecisc commented Jan 11, 2024

Those were options we talked about with Pablo and I'm fine with both. We just did not start this work because it requires time

@Ducasse
Copy link
Contributor

Ducasse commented Jan 11, 2024

I like the second one more than the first one (did not think a lot about them).
We could still build on the second one to have a tool checking that the trait is complete.
We would have to go from the class to its traits check all the methods to identify the explicit requirements. So it looks ok to me. I prefer that than being forced to check all the subclasses.

@tinchodias tinchodias closed this Jan 29, 2024
@tinchodias tinchodias deleted the speedup-shapes branch January 29, 2024 21:54
@tinchodias tinchodias restored the speedup-shapes branch January 29, 2024 22:09
@jecisc jecisc reopened this Jan 29, 2024
@tinchodias tinchodias changed the base branch from masterTmp to master January 29, 2024 23:38
@tinchodias tinchodias closed this Jan 29, 2024
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

Successfully merging this pull request may close these issues.

7 participants