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 small bug with FFI calls when passing 'NULL' as argument to the C function #16562

Merged
merged 2 commits into from
May 30, 2024

Conversation

doste
Copy link
Contributor

@doste doste commented May 2, 2024

Now it can infer its type correctly as 'void*'

… C function to call. Now it can infer its type correctly as 'void*'
There is no ambiguity.
"

(aFFIValueArgument value isNil
Copy link
Member

@astares astares May 14, 2024

Choose a reason for hiding this comment

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

This give a lint issue and to make it easier it could be rewritten as

	(#(nil 'nil' 'NULL') includes: aFFIValueArgument value)
		 ifTrue: [ ^ (aResolver resolveType: #'void *') ].
	 ...

avoiding the critique to appear

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @astares, I applied your suggestion using the github review tool.

Let's wait for the CI

@astares
Copy link
Member

astares commented May 14, 2024

Also this PR should be against Pharo13 branch (with a possible backport to P12)

@tesonep tesonep changed the base branch from Pharo12 to Pharo13 May 15, 2024 11:53
@tesonep
Copy link
Collaborator

tesonep commented May 15, 2024

I changed the base of the PR, thanks for the catch @astares

@guillep guillep merged commit 706e6ac into pharo-project:Pharo13 May 30, 2024
2 checks passed
@guillep
Copy link
Member

guillep commented May 30, 2024

Thanks @doste for the PR and @astares for the review.
We are going to discuss if this is worth backporting to P12

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

5 participants