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.

Sign up for GitHub

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

Jump to bottom

Cleanup OpalCompiler & CompilationContext #13423

Merged
merged 8 commits into from
Apr 18, 2023
Merged

Cleanup OpalCompiler & CompilationContext #13423

merged 8 commits into from
Apr 18, 2023

Conversation

privat
Copy link
Contributor

@privat privat commented Apr 12, 2023 •
edited
Loading

In the OpalCompiler ecosystem, cohesion was very low and coupling very high.
In order to pass around information (and worse, store it is some escaping objects like AST) between the various components, a god-dataclass (two smells in one!) called CompilationContext was used.

After a few weeks of intense cleaning, the cohesion is higher and coupling lower, and some data stored in CompilationContext is now used by only a single class: OpalCompiler. Therefore, the present PR move this data to the class that has the responsibility to use them.

  • permitFaulty, permitUndeclared and failBlock. As they are now only used after the parse/semantic analysis to decide to abort the compilation pipeline or to continue.
  • logged (boolean flag) used to fire announcement on evaluation or installation

Beside the increase of code quality, this has the advantage of reducing the memory footprint caused by escaping CompilationContext. (maybe it's not that much, it's hard to quantify).

What remain (for the moment) in CompilationContext are:

  • All things related to scope and environment, including the badly named noPattern
  • All things related to code optimization
  • All "class methods" i.e. methods that return a class. It's used by reflectivity/metalinks
  • Parse and transform plugins. Moving them breaks things. Need to investigate

jecisc
jecisc approved these changes Apr 13, 2023
View reviewed changes
Copy link
Member

@jecisc jecisc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code seems way nicer

@privat privat mentioned this pull request Apr 13, 2023
Open
78 tasks
@Ducasse
Copy link
Member

Ducasse commented Apr 13, 2023

Sweet! I love that.

@jecisc
Copy link
Member

jecisc commented Apr 13, 2023

The formatter seems impacted by those changes:

#> was sent to nil

UndefinedObject(Object)>>doesNotUnderstand: #>

UndefinedObject>>doesNotUnderstand: #>

EFFormatter>>newLines:

[ 

			self newLines: (aMethodNode comments 

									ifEmpty: [ self numberOfNewLinesAfterMethodSignature ] 

			 						ifNotEmpty: [ self numberOfNewLinesAfterMethodSignatureWithMethodComment ]).

			self formatMethodCommentFor: aMethodNode.

			self formatPragmasFor: aMethodNode.

			self visitNode: aMethodNode body ] in EFFormatter>>formatMethodBodyFor: in Block: [ ...

FullBlockClosure(BlockClosure)>>ensure:

EFFormatter>>indent:around:

EFFormatter>>indentAround:

EFFormatter>>formatMethodBodyFor:

EFFormatter>>visitMethodNode:

RBMethodNode>>acceptVisitor:

EFFormatter(RBProgramNodeVisitor)>>visitNode:

[ 

			needsParenthesis

				ifTrue: [ codeStream nextPutAll: self spacesInsideParenthesesString ].

			super visitNode: aNode.

			(self formatCommentCloseToStatements or: [ aNode isMethod or: [ aNode isSequence or: [ aNode isBlock ] ] ])

				ifFalse: [  self spaceAndFormatComments: aNode. ].

			needsParenthesis

				ifTrue: [ codeStream nextPutAll: self spacesInsideParenthesesString ] ] in EFFormatter>>visitNode: in Block: [ ...

EFFormatter>>bracketWith:around:indentExtraSpaces:

EFFormatter>>visitNode:

EFFormatter>>format:

RBMethodNode(RBProgramNode)>>formattedCodeIn:

RBMethodNode(RBProgramNode)>>formattedCode

[aMethod origin compile: aMethod ast formattedCode classified: aMethod protocol] in [

		rewriteRule := self rewriterClass new

			replace: rule key with: rule value.

		(rewriteRule executeTree: node)

			ifFalse: [ ^ self signal ].

		node replaceWith: rewriteRule tree.

		Author

@jecisc jecisc added the Status: Need more work The issue is nearly ready. Waiting some last bits. label Apr 13, 2023
@privat privat changed the base branch from Pharo12 to Pharo11 April 13, 2023 17:41
@privat privat changed the base branch from Pharo11 to Pharo12 April 13, 2023 17:42
@privat privat added Status: Tests passed please review! and removed Status: Need more work The issue is nearly ready. Waiting some last bits. labels Apr 14, 2023
@privat
Copy link
Contributor Author

privat commented Apr 14, 2023

I rebased the PR to let plugins in place. For some reason is breaks FFI on the CI but not on my machine.
So I need to investigate more.

Tests are now ok (beside #13446)

@privat privat changed the base branch from Pharo12 to Pharo11 April 14, 2023 19:58
@privat privat changed the base branch from Pharo11 to Pharo12 April 14, 2023 19:58
@privat privat changed the base branch from Pharo12 to Pharo11 April 14, 2023 21:26
@privat privat changed the base branch from Pharo11 to Pharo12 April 14, 2023 21:26
@privat privat changed the base branch from Pharo12 to Pharo11 April 14, 2023 21:29
@privat privat changed the base branch from Pharo11 to Pharo12 April 14, 2023 21:29
@MarcusDenker MarcusDenker merged commit 1301b93 into pharo-project:Pharo12 Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jecisc jecisc jecisc approved these changes

@MarcusDenker MarcusDenker MarcusDenker approved these changes

Assignees
No one assigned
Labels
Status: Reviewed and Ready to Integrate
Projects
Compiler and AST
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@privat @Ducasse @jecisc @MarcusDenker