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

Implement GRPlatform >> #thisContext on GemStone #40

Closed
marschall opened this issue Sep 6, 2017 · 8 comments
Closed

Implement GRPlatform >> #thisContext on GemStone #40

marschall opened this issue Sep 6, 2017 · 8 comments
Assignees

Comments

@marschall
Copy link
Contributor

marschall commented Sep 6, 2017

We now have a #thisContext method for dialects that do not have a thisContext variable.

In order to get the tests running and the #stackDepth method working we need the following API:

  • #sender: Answer the context that sent the that created the receiver. Can either be a block or or method context. Is nil for the topmost stack frame.

In order to get WAPharoWalkback to work we need the following API:

  • #receiver: Answer the receiver object of the message receiver. Can be a block object.
  • #printStringLimitedTo:
  • #tempAt: Answer the temporary variable at the given index.

We used to send #tempScopedNames but that seems to be gone.
We used to send more messages but this seems to be the only part of the Pharo context API that is guaranteed to work.

There are references to thisContext in methods named evaluate that read something like

evaluate
	"GemStone does not have a thisContext variable..."

	^input evaluateInContext: self object symbolList: GsSession currentSession symbolList

I left out what we need for continuations since they're going to be different for GemStone anyway.

Since they are GemStone specific anyway I don't know how much replacing these makes sense.

@marschall marschall added this to the 1.3.2 milestone Sep 6, 2017
@marschall marschall modified the milestones: 1.3.3, 1.3.2 Sep 9, 2017
@jbrichau jbrichau modified the milestones: 1.3.3, 1.3.4 Oct 4, 2017
@jbrichau
Copy link
Member

jbrichau commented Oct 4, 2017

Hi @dalehenrich,
Just a gentle question on this issue if it is possible to expect this sometime in the near future in Grease? Just to determine how to proceed with Seaside 3.4

@dalehenrich
Copy link
Member

Are you waiting for me to do the implementation?

@jbrichau
Copy link
Member

jbrichau commented Oct 4, 2017

Oh, I thought @marschall told me you had an idea on how to do that. I just assumed this was discussed at ESUG. If this was not the case, I'm curious to hear your thoughts on how to approach this in GemStone.

@dalehenrich
Copy link
Member

I think there was only one spot where ‘thisContext’ was being used so a method wasn’t needed ... If there is more than one spot then the method can be added ... with regards to continuations ‘thisContext’ isn’t the only reason we don’t have a common implementation

@jbrichau
Copy link
Member

jbrichau commented Oct 7, 2017

Ok, I took some time to read up on the issue and figure out why this idea exists and what your response to my previous question actually means :)
As I understand it, a thisContext method would be useful in WAGemStoneInspector and WAObjectLogInspector, so I agree it's not really needed as the impact on the code is rather limited.

Just to give it a try, I tried implementing the thisContext method as follows, but that did not satisfy the test at this time. By inspecting the resulting object, I do think the implementation is correct, but the test uses object identity to compare the resulting context objects and I have the impression the different invocations of GsProcess fromLevel: 1 result in different instances being created, which is not what thisContext does in Pharo.

thisContext
   ^ (GsContext fromLevel: 1) sender sender

So... @marschall... do we need object identity comparison? @dalehenrich: does that sound correct to you?

@marschall
Copy link
Contributor Author

@jbrichau I don't think we need the object identity, I just needed to write a minimal test and I wanted to express the contract of #sender, if you can come up with a better test, feel free to change the test

@jbrichau jbrichau removed this from the 1.3.5 milestone Nov 19, 2017
@jbrichau
Copy link
Member

TODO: Need to move GsContext class from Seaside-GemStone-Core to Grease.

@jbrichau
Copy link
Member

For Grease 1.4.0, I only moved GsContext from Seaside to Grease. We can take it from there but I don't want to keep us from releasing Seaside at this time.

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