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 a lot of unused expression #832

Merged
merged 38 commits into from
Aug 14, 2024

Conversation

RenaudFondeur
Copy link
Contributor

@RenaudFondeur RenaudFondeur commented Jul 5, 2024

add a visitor in Slang to suppress unused expressions after inlining occurs

@PalumboN
Copy link
Collaborator

PalumboN commented Jul 5, 2024

Hi @RenaudFondeur !

I cannot review this PR because all the files were changed by Tonel :(

I was checking the new tests, they looks ok.

suppress unused expression including some more complex than just variables and constants.

Which ones? Those tests show that after inlining there are still some unused local assignments and unnecessary ifs.

@RenaudFondeur
Copy link
Contributor Author

RenaudFondeur commented Jul 5, 2024

hello @PalumboN !

The tests missing are the cases where the variable need to be retained because the method to be inlined is either in a return, an assignment or the receiver/argument of a send.

Sometimes inlining doesn't put a goTo node in the conditional send meaning it can become empty during the check, in this case the check either suppress it or reduce it (for example ifTrue:ifFalse: -> ifFalse: if the ifTrue: is empty).

The unused variable can also arrive in the check after getting some transformation like a cast or becoming a function call if it is an instance variable.

…eUnusedNodesInBranch: .

supress harmonizeSignedAndUnsignedTypesIn: , it has no sender and look exactly like an old version of  harmonizeReturnTypesIn: now in CCodeGenerator
when inlining happens, we produce a copy of the method's AST but we only ensure that the children are correct meaning we cannot iterate the AST from bottom top anymore.
@guillep guillep merged commit d9e7e20 into pharo-project:pharo-10 Aug 14, 2024
1 of 2 checks passed
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.

3 participants