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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate if locals/args exist when adding a type declaration #603

Merged

Conversation

ivojawer
Copy link
Collaborator

fix #598

Adds a validation at TMethod >> declarationAt: aVariableName put: aDeclaration

Removed unused type declarations and then generated the VM C Code and compiled succesfully 馃殌

@guillep
Copy link
Member

guillep commented May 24, 2023

there seems to be a broken part of the build because of the PR :)

Copy link
Collaborator

@PalumboN PalumboN left a comment

Choose a reason for hiding this comment

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

Cool! 馃殌

I left some questions and comments to check.

Now the CI is broken 馃憥

Comment on lines +7 to +11
{ #category : #translation }
SLBasicTestDeclarationClass class >> typeForSelf [

^#implicit
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary? Is it not always implicit (except for structs)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The inherited behaviour from SlangClass is to return nil. Or are you saying that at some point that nil defaults to #implicit?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just asking for understanding, because this definition is in the tests, but maybe could be in a more general place? I don't know.
Anyway, it is out of the scope of this PR, so we can continue with our life.

smalltalksrc/Slang/TMethod.class.st Outdated Show resolved Hide resolved
smalltalksrc/Slang/TMethod.class.st Outdated Show resolved Hide resolved
@@ -179,7 +179,6 @@ ComposedImageReader >> readPermanentSpaceDataFromImage: imageFileName startingAt
ComposedImageReader >> readPermanentSpaceFromImageFile: imageFileName header: aHeader [

<inline: false>
<var: #imageFile type: #sqImageFile>
Copy link
Collaborator

Choose a reason for hiding this comment

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

馃憦

Copy link
Member

@guillep guillep left a comment

Choose a reason for hiding this comment

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

Little comments over here and there

smalltalksrc/Slang-Tests/SLTestDeclarations.class.st Outdated Show resolved Hide resolved
smalltalksrc/Slang/TMethod.class.st Outdated Show resolved Hide resolved
@PalumboN PalumboN added the slang label Jun 12, 2023
@PalumboN
Copy link
Collaborator

PalumboN commented Jun 12, 2023

Failing tests are not related. Windows is not compiling, but neither is in the main branch.

I think that it is mergeable 馃憤

@guillep
Copy link
Member

guillep commented Jun 12, 2023

I just fixed a merge conflict, I'll wait the CI to see if I did well :)

@guillep
Copy link
Member

guillep commented Jun 12, 2023

Tests are green!! Thanks!!

@guillep guillep merged commit 0069deb into pharo-project:pharo-12 Jun 12, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants