Skip to content

Use C++-style initialization for zeroed array type #19804

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

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

cjjdespres
Copy link
Contributor

The {} initializer will zero-initialize a variable of the array type jmp_buf. Clang otherwise complains about the use of 0 to zero-initialize this array type.

The {} initializer will zero-initialize a variable of the array type
`jmp_buf`. Clang otherwise complains about the use of `0` to
zero-initialize this array type.

Signed-off-by: Christian Despres <[email protected]>
@cjjdespres
Copy link
Contributor Author

Attn @mpirvu.

@@ -55,7 +55,7 @@ ffiCallWithSetJmpForUpcall(J9VMThread *currentThread, ffi_cif *cif, void *functi
ffiCallWithSetJmpForUpcall(J9VMThread *currentThread, ffi_cif *cif, void *function, UDATA *returnStorage, void **values)
#endif /* FFI_NATIVE_RAW_API */
{
jmp_buf jmpBufferEnv = {0};
jmp_buf jmpBufferEnv = {};
Copy link
Contributor

@mpirvu mpirvu Jul 3, 2024

Choose a reason for hiding this comment

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

Reading the documentation I see that { } was introduced in C23: https://en.cppreference.com/w/c/language/array_initialization

Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot find when initializing an array with {} in c++ started to be valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should omit = to use a form that has been valid since C++11?

	jmp_buf jmpBufferEnv {};

Copy link
Member

@pshipton pshipton Jul 3, 2024

Choose a reason for hiding this comment

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

Whatever you decide on, pls try it in a vmfarm build on Linux PPC BE, AIX and z/OS before merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to suggest that until I noticed that this code is guarded by #if JAVA_SPEC_VERSION >= 16.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the syntax I have is okay even before C++-11, judging from the articles initialization and aggregate_initialization - it's either aggregate initialization or list initialization of an aggregate type (an array type). I don't think the C standard applies here.

The page zero_initialization does point out that jmp_buf jmpBufferEnv {}; will work, as you say.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you agree with my suggestion (#19804 (comment)), but this was merged without that change being made. What happened?

@mpirvu mpirvu self-assigned this Jul 3, 2024
@mpirvu mpirvu added the comp:vm label Jul 3, 2024
@mpirvu
Copy link
Contributor

mpirvu commented Jul 3, 2024

@keithc-ca Since this is vm code, I would appreciate a review from you. Thanks

@mpirvu
Copy link
Contributor

mpirvu commented Jul 5, 2024

jenkins compile all jdk8,jdk21

@mpirvu mpirvu merged commit 526aad5 into eclipse-openj9:master Jul 8, 2024
@mpirvu
Copy link
Contributor

mpirvu commented Jul 8, 2024

I think the syntax I have is okay even before C++-11, judging from the articles initializatio

I interpreted this as being conforming c++ code and therefore it should compile on all platforms. It did compile successfully on all platforms that are tested externally.

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.

4 participants