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

Use C99 named initialisers in core structs that define MGVTBLs #22086

Open
wants to merge 4 commits into
base: blead
Choose a base branch
from

Conversation

leonerd
Copy link
Contributor

@leonerd leonerd commented Mar 18, 2024

Since most Magic vtables do not provide every function, using C99 named initialisers allows us to more easily specify which ones are present, so readers don't have to count commas. Additionally provides a layer of robustness in case more functions are added in future.

(No rush before 5.39.9 but I would like to test this out at least, sometime before too long)

regen/mg_vtable.pl Outdated Show resolved Hide resolved
@Leont
Copy link
Contributor

Leont commented Mar 18, 2024

I thought we couldn't use this in public headers because C++?

@leonerd
Copy link
Contributor Author

leonerd commented Mar 18, 2024

I thought we could use this in public headers because C++?

Ugh; it's mg_vtable.h, indeed. Does that still count? Even if we can't use it there, I'd still like to do the .c files.

@ilmari
Copy link
Member

ilmari commented Mar 18, 2024

I thought we could use this in public headers because C++?

Ugh; it's mg_vtable.h, indeed. Does that still count? Even if we can't use it there, I'd still like to do the .c files.

It gets included in perl.h, so yes. But having it for the hand-written ones in .c files is still a big win.

@tonycoz
Copy link
Contributor

tonycoz commented Mar 18, 2024

I thought we could use this in public headers because C++?

Ugh; it's mg_vtable.h, indeed. Does that still count? Even if we can't use it there, I'd still like to do the .c files.

Another option would be to generate a separate mg_vtable.c with the initializations.

The DOINIT code doesn't appear usable outside of the executable/libperl.so since the magic functions are marked hidden.

@jkeenan
Copy link
Contributor

jkeenan commented Mar 18, 2024

@leonerd, Given where we are in our annual production cycle, are you thinking of this for 5.40 or 5.42?

@leonerd leonerd force-pushed the named-initialisers-for-mgvtbl branch from 2bb5119 to 7b38f46 Compare March 19, 2024 08:07
@leonerd
Copy link
Contributor Author

leonerd commented Mar 19, 2024

@leonerd, Given where we are in our annual production cycle, are you thinking of this for 5.40 or 5.42?

Oh it doesn't need to be in 5.40 at all; it isn't doing any kind of user-benefit change. It's purely tidying up the code internally for our benefit. It can wait for 5.42, I just happen to have started writing it now.

@leonerd leonerd force-pushed the named-initialisers-for-mgvtbl branch 2 times, most recently from cf6347e to 678aa24 Compare March 19, 2024 09:10
@ilmari
Copy link
Member

ilmari commented Mar 19, 2024

The DOINIT code doesn't appear usable outside of the executable/libperl.so since the magic functions are marked hidden.

Looking more closely at things, DOINIT is only defined in INTERN.h, which is only included in globals.c, so even though mg_vtable.h is included in perl.h, that only causes the extern declarations to be visible outside globals.c, so we could change the generated code after all.

Since most Magic vtables do not provide every function, using C99 named
initialisers allows us to more easily specify which ones are present, so
readers don't have to count commas. Additionally provides a layer of
robustness in case more functions are added in future.
@leonerd leonerd force-pushed the named-initialisers-for-mgvtbl branch from 678aa24 to 625cd98 Compare March 19, 2024 11:54
These named initialisers are not permitted by C++ compilers using
standards older than C++20, but fortunately even though they appear in
mg_vtable.h and hence perl.h, they are hidden behind an `#ifdef` that is
only defined when compiling `globals.c`, so this should be fine.
@leonerd leonerd added the defer-next-dev This PR should not be merged yet, but await the next development cycle label Mar 19, 2024
C99 allows trailing commas in on struct and table initialisers, so get
rid of the code to avoid it in PL_magic_vtables and adjust the _names
code to match.

We don't add it to the enum, since magic_vtable_max is always the last
element, so there won't be any diff noise from adding new elements
after it.
Comment on lines +108 to +118
[want_vtbl_arylen] = "arylen",
[want_vtbl_arylen_p] = "arylen_p",
[want_vtbl_backref] = "backref",
[want_vtbl_checkcall] = "checkcall",
[want_vtbl_collxfrm] = "collxfrm",
[want_vtbl_dbline] = "dbline",
[want_vtbl_debugvar] = "debugvar",
[want_vtbl_defelem] = "defelem",
[want_vtbl_destruct] = "destruct",
[want_vtbl_env] = "env",
[want_vtbl_envelem] = "envelem",
Copy link
Contributor

Choose a reason for hiding this comment

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

Array designated initializers are not C++ compatible, and since we have people who build perl with a C++ compiler, it's excluded by the guide in perlhacktips.pod.

g++ supports it as an extension. but MSVC doesn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defer-next-dev This PR should not be merged yet, but await the next development cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants