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

delay size pragma for generic types, use it for C++ Atomic[T] #24204

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Sep 30, 2024

fixes #24159, refs #15928 etc, refs #7674 (fixed but not tested)

When a size pragma is applied to a generic type, the pragma is untouched and the type is marked as having "unresolved properties". This is regardless of whether the pragma actually uses any generic parameters, because by the time the pragma is being processed, generic parameters aren't in scope yet. For pragma processing to know that the type is generic, the forward type is temporarily given the flag tfHasMeta to mark that it has generic parameters.

When this type undergoes generic instantiation, the pragma is found again from the type symbol AST (not sure if this is reliable), the value is instantiated, and the new size is applied to the type. Other pragmas can opt in to this later, currently just size is checked.

This allows us to do things like size: sizeof(T) where T is a generic parameter, and this is now used instead of completeStruct and the fake raw field added in #15928 for the size of Atomic[T] on C++. This fixes the issues related to the raw field like #24159. The restriction of only the values 1, 2, 4 and 8 being allowed for the size pragma is also removed for imported types for this to be possible.

There are a couple reasons I wasn't sure about this solution and some things I'm still not sure about:

  • Going through the symbol pragma AST seems dubious but it's usually what we do in cases like this (example being custom deprecated messages), just never in a case that actually has an effect on the type system. I didn't want to complicate the type graph even more by adding a dedicated field that's redundant with pragmas. In the future maybe we can use the annex field of PSym to store/access pragmas including custom pragmas more efficiently in general, , but then there still needs to be a way to access this in macros.
  • I thought about ways to still add the generic parameters to scope early somehow so that we only delay the pragma if unresolved parameters are used, but that seems more prone to bugs and more of an effort. Niche cases like forward types as constraints need to be handled for example.
  • I'm not sure if the alignment calculation is right. There doesn't seem to be an alignas pragma either. I thought the align pragma did this, which is why I initially generalized this for all "properties".
  • This isn't very suitable for backporting, so there's the question of what to do about invalid C++ codegen in --mm:refc with Thread/Atomic/createThread #24159 on the 2.0 branch, since fixes #9940; genericAssign does not take care of the importC variables in refc [backport] #23761 which caused it was backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invalid C++ codegen in --mm:refc with Thread/Atomic/createThread
1 participant