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

[fix] separation between isTemp from the compiler/AST pov #6013

Merged
merged 2 commits into from Mar 27, 2020

Conversation

hogoww
Copy link
Contributor

@hogoww hogoww commented Mar 27, 2020

isTemp from the compiler pov == isArgOrTemp (because the VM treats them the same way)
We now have 2 different perspective for isTemp.
Compiler tests passing, we'll see with the CI for the others.

…ne (which is the VM view of temp) with isArgOrTemp
@hogoww
Copy link
Contributor Author

hogoww commented Mar 27, 2020

failures are unrelated, see 6023

@MarcusDenker MarcusDenker merged commit b536bfb into pharo-project:Pharo9.0 Mar 27, 2020
@dionisiydk
Copy link
Contributor

Maybe we can keep isTemp for all kind of temps. But in addition we can add isArgument.

@hogoww
Copy link
Contributor Author

hogoww commented Mar 27, 2020

#isArgument is already there (AST point of view)
also, #isArg is there (compiler POV)

Also, isTemp from the compiler POV is arg + temp,
whereas from an AST point of view it's only Temporary nodes.

@dionisiydk
Copy link
Contributor

Oh, It looks really messy.
We should not have different API depending on POV.

The binding object is always there whatever we do with semantic analysis.
So can we just keep binding based version and remove the rest? Why it is bad idea?

@hogoww
Copy link
Contributor Author

hogoww commented Mar 27, 2020

I assume it's because they both got developed concurrently, so some stuff got intertwined.
Technically, RBVariableNode >> isTemp/isArg & co are extention methods from the compiler, but they are still used as regular methods by users (I'm part of them)

"The binding object is always there whatever we do with semantic analysis."
This is true when you take a compiled method, this is might not be when you have a tool that is modifying the AST (which I do), without (at least directly) recompiling it.
For example I take do:
m := aCompiledMethod ast.
m firstModifyStep.
m secondModifyStep.
m compile. "maybe, not always"

And when I move a node to be temporary from argument for example, I loose the binding (which I don't care about). This prevents me to use those selectors without custom overrides.

@dionisiydk
Copy link
Contributor

See my comment in another PR on idea to rely only on bindings. I think it will allow to simplify such operations.

And when I move a node to be temporary from argument for example, I loose the binding (which I don't care about). This prevents me to use those selectors without custom overrides.

Here it could be something like:

RBVariableNode>>beTempVariable
      binding := OCTempVariable inScope: binding scope.

I see that now #parseTree does not add any binding to variable nodes. But we can have something as default (OCUnresolvedVariable).

Now I also wonder why we use a #binding name here instead of #variable (all OC hierarchy is about variables).
It would be much simpler to have RBVariableNode>>variable.

@dionisiydk
Copy link
Contributor

But I understand now we have what we have. So what I wrote is just ideas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants