Skip to content

[FFI/Test] Test suites intended for the duplicate ffi_type for struct #19922

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

Conversation

ChengJin01
Copy link

@ChengJin01 ChengJin01 commented Jul 26, 2024

The changes add new test suites which are used to verify
the code that avoids creating the duplicate ffi_type for
the same struct for arguments/return type in downcall.
Also, these tests must work for the existing FFI specific
implementation.

Related: #19714

Signed-off-by: ChengJin01 [email protected]

@ChengJin01 ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 4700c46 to 356fb45 Compare July 26, 2024 04:23
@ChengJin01 ChengJin01 added comp:test project:panama Used to track Project Panama related work labels Jul 26, 2024
@ChengJin01
Copy link
Author

The new test suites are verified in personal builds on all supported platforms.

Reviewer: @tajila, @keithc-ca
FYI: @pshipton

@ChengJin01 ChengJin01 requested review from tajila and keithc-ca July 26, 2024 04:25
@ChengJin01 ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch 7 times, most recently from d180da5 to 9b6226a Compare July 30, 2024 03:26
@ChengJin01 ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 9b6226a to ee37591 Compare July 30, 2024 19:54
@ChengJin01
Copy link
Author

@keithc-ca, is there anything else to be addressed in this PR?

@keithc-ca
Copy link
Contributor

Yes, I have been reviewing the current state of this; I will share my comments tomorrow.

Copy link
Contributor

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

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

I've highlighted a number of unused imports; please remove those from other packages as well.

@ChengJin01 ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from ee37591 to 63a6272 Compare August 1, 2024 20:41
@keithc-ca
Copy link
Contributor

The description says "avoids the duplicate ffi_type for struct"; please explain what that means.

@ChengJin01 ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch 2 times, most recently from 2f0ad43 to 2f3fbe8 Compare August 2, 2024 14:26
@ChengJin01
Copy link
Author

ChengJin01 commented Aug 2, 2024

The description says "avoids the duplicate ffi_type for struct"; please explain what that means.

As explained in the description at #19714, we will need to optimize the existing code to avoid the creating duplicate ffi_type for the same struct in argument/return type in downcall (already updated the commit description to clarify that).

@ChengJin01 ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 2f3fbe8 to 9ec3dc2 Compare August 2, 2024 19:44
@ChengJin01
Copy link
Author

@keithc-ca, I've gone through all related test suites with unused imports & displaced imports fixed. Please help cross-check whether anything else I ignored the latest update.

@ChengJin01 ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 9ec3dc2 to 35d9f5d Compare August 2, 2024 21:06
Copy link
Contributor

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

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

Each of the comments from this round apply to several files other than those explicitly mentioned: please address those issues throughout.

@ChengJin01 ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 35d9f5d to 59ae9cb Compare August 7, 2024 16:04
@ChengJin01 ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from 59ae9cb to b642c47 Compare August 7, 2024 20:13
The changes add new test suites which are used to verify
the code that avoids creating the duplicate ffi_type for
the same struct for arguments/return type in downcall.
Also, these tests must work for the existing FFI specific
implementation.

Related: eclipse-openj9#19714

Signed-off-by: ChengJin01 <[email protected]>
@ChengJin01 ChengJin01 force-pushed the ffi_avoid_duplicating_ffi_type_struct_downcall_2_test_suites branch from b642c47 to 80abde0 Compare August 7, 2024 21:26
@keithc-ca
Copy link
Contributor

Jenkins test sanity zlinux jdk17,jdk21,jdk22

@keithc-ca
Copy link
Contributor

@tajila Could you review this please?

@tajila tajila merged commit 0ed928f into eclipse-openj9:master Aug 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:test project:panama Used to track Project Panama related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants