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

Should arguments be assignable? #7

Closed
smarr opened this issue Nov 1, 2018 · 8 comments
Closed

Should arguments be assignable? #7

smarr opened this issue Nov 1, 2018 · 8 comments
Labels
bug Fixes an issue, incorrect implementation question

Comments

@smarr
Copy link
Member

smarr commented Nov 1, 2018

In SOM-st/TruffleSOM#17, @gpummer found that TruffleSOM departed from SOM semantics.
SOM allows assignment to arguments, while TruffleSOM doesn't.

@krono @mhaupt @charig, any opinion on this?

If I am not mistaken, most Smalltalk do not allow assignment to arguments.
And personally, I don't think there is a good reason for them to be assignable.

@charig, what did you do in Mate?

@smarr smarr added the question label Nov 1, 2018
@krono
Copy link
Member

krono commented Nov 2, 2018 via email

@smarr
Copy link
Member Author

smarr commented Nov 2, 2018

I think, I'll consider this a bug, and define the desired behavior of SOM to be that arguments are read-only.

Though, testing this is going to be not straight forward. Will need some external testing infrastructure, since parser errors are usually fatal, I think.

Thanks @gpummer for finding this!

@smarr smarr added the bug Fixes an issue, incorrect implementation label Nov 2, 2018
@charig
Copy link

charig commented Nov 2, 2018

I almost did not change the parser. So I guess in Mate the behavior is the same. I'd have to check.
This said, it is clearly not the "intended behaviour". In Mate there are capabilities for redefining reading & writing of locals, but for arguments only reading.
I neither see any benefits of assigning to args.

@mhaupt
Copy link

mhaupt commented Nov 2, 2018 via email

@smarr
Copy link
Member Author

smarr commented Nov 2, 2018

@mhaupt good point.
Indeed, there is a bytecode for it: https://github.com/SOM-st/som-java/blob/master/src/som/interpreter/Bytecodes.java#L44

public static final byte POP_ARGUMENT     = 10;

And it does

  private void doPopArgument(final int bytecodeIndex) {
    // Handle the POP ARGUMENT bytecode
    getFrame().setArgument(
        getMethod().getBytecode(bytecodeIndex + 1),
        getMethod().getBytecode(bytecodeIndex + 2),
        getFrame().pop());
  }

https://github.com/SOM-st/som-java/blob/master/src/som/interpreter/Interpreter.java#L118

The easiest change of course would be to fix TruffleSOM, and add a test.
Hmmm.

smarr added a commit that referenced this issue Nov 11, 2018
This is the desired semantics as discussed in issue #7.

Signed-off-by: Stefan Marr <[email protected]>
@smarr
Copy link
Member Author

smarr commented Nov 11, 2018

Ok, #8 added a tests to ensure that the arguments are assignable as they are in the original SOM.

@smarr smarr closed this as completed Nov 11, 2018
@gpummer
Copy link

gpummer commented Nov 15, 2018

I will try to add the required functionality to TruffleSOM. At the moment it seems somewhat difficult to me. But that is fine, SOM is for learning:)

@smarr
Copy link
Member Author

smarr commented Nov 15, 2018

@gpummer happy to talk you through if you want help :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes an issue, incorrect implementation question
Projects
None yet
Development

No branches or pull requests

5 participants