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

ClassDescription>>protocols should return protocols #13674

Merged

Conversation

jecisc
Copy link
Member

@jecisc jecisc commented May 9, 2023

#protocols currently returns a list of protocol names but now we have #protocolNames for that.

This changes this behavior to return real protocols and adapt the system to use that.

This is the stepping stone in order to inline further the protocol management from ClassOrganization since almost everything relies on this method.

Fixes #10856

@MarcusDenker MarcusDenker added the Status: Need more work The issue is nearly ready. Waiting some last bits. label May 11, 2023
@MarcusDenker
Copy link
Member

Interesting... MCClassDefinition>>#createClass skips Context explicitly:

	"Ignore Context definition because of troubles with class migration on bootstrapped image. Temporary solution."
	name = #Context
		ifTrue: [ Context comment = comment 
						ifFalse: [ Context comment: comment stamp: commentStamp ].
					^ self ].

Maybe we can try to add FullBlockClosure here, too, as the problem is that it tries to load a new definition for class FullBlockClosure and then trying to update exiting instances?

Another thing to check: why does the classbuilder update the instances if the shape if the class is not different? This is not needed just to load set the comment.

@jecisc
Copy link
Member Author

jecisc commented May 19, 2023

That looks promising!

I'll add that to the PR when I'll have the time

@jecisc
Copy link
Member Author

jecisc commented May 21, 2023

The fix seems to work! but this PR has failing tests. I'll do the fix in a simpler PR so that we can integrate it if the CI is green and merge it in all PRs that fails because of this :)

@MarcusDenker
Copy link
Member

Now we can look at the image...

indeed there are problems due to not recompiling:

CompiledCode subclasses first. "CompiledBlock"
CompiledCode subclasses first == CompiledBlock "false"

So we have to recompile the class to fix that.

Now for CompiledBlock instances, recompiling CompiledBlock in the full-image reload triggers a become: of all instances,
that failed. We can now look at them to see what might be wrong.

If we inspect "CompiledBlock allInstances", we see directly at the start we see instances that can not be decompiled.

But we have those in a normal image, too (not nice... we need to check that).

But when looking at the raw view, we see that most of them have a 4byte empty trailer.

In the normal image, the BC has no four 0, so maybe that is the problem, someone creates CompiledBlock instances with a 4 byte trailer.

The question is, why is that a problem just with that PR?

@jecisc
Copy link
Member Author

jecisc commented May 22, 2023

This is indeed really mysterious

@jecisc jecisc added Status: Tests passed please review! and removed Status: Need more work The issue is nearly ready. Waiting some last bits. labels May 30, 2023
@jecisc
Copy link
Member Author

jecisc commented May 30, 2023

This PR finally passes! This will be a good step toward the inlining of ClassOrganization and this will allow me to start the next steps

@MarcusDenker MarcusDenker merged commit ec4f3d2 into pharo-project:Pharo12 May 30, 2023
1 of 2 checks passed
@jecisc jecisc deleted the protocols-should-return-protocols branch May 30, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Cleanup Behavior>>#protocols should return a Protocol, not symbol
2 participants