-
Notifications
You must be signed in to change notification settings - Fork 81
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
Initialize first field in named union constructors #784
Conversation
…itialized" This reverts commit 6c79e2c.
This allows job_spec_data fields embedded in other types to be initialized to -1 as expected
@@ -677,7 +677,13 @@ sub emit_struct_fields($$;%) { | |||
} 'fields-' . $fields_group; | |||
|
|||
# Needed for unions with fields with non-default ctors (e.g. bitfields) | |||
emit "$name(){memset(this, 0, sizeof($name));}"; | |||
emit_block { | |||
local $in_union = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this variable unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's used to tell render_field_init
that it's rendering fields from a union, and is necessary here to get the behavior we want.
Remember that in Perl, local
variables are dynamically, not lexically, scoped (see What is the difference between my and local in Perl?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's read here in render_field_init
but is not set there:
Line 409 in 833a1fd
local $in_union = $in_union || $is_union; |
Probably an oversimplification: I've been thinking of local
vars as additional context args that get pushed on the stack and get restored when they go "out of scope". Inspecting $is_union
outside of this emit_block
will show it still set to 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they're actually pushed into the current block's dynamics symbol table, which remains visible during a subroutine call, so any subroutine called will also see them (unless shadowed). lexically scoped my
locals, however, are only visible within the lexical block (and any enclosed blocks) in which they're created.
it's not so much that they get "restored" but rather that defining a symbol within a block shadows any definition that might exist in a block further up. when the block ends, its symbol tables are disposed (each block effectively has two, one for dynamic scope and one for lexical scope), and any prior definition that was being shadowed is thereby unshadowed and thus once again visible. every block carries with it a pointer to the lexical context in which it was defined (a closure), and the current dynamic symbol context is used to initialize a new dynamic symbol context whenever execution enters a new block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank for the explanation. my perl fu is low
This looks good to me in general, but I notice that some of the fields are being initialized to -1 and I don't understand why this happens. Why is, for example, |
That's handled here in Lines 423 to 428 in 833a1fd
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. i've reviewed both the diff provided as well as run a local build with this, and it appears to work as intended.
This allows
job_spec_data
fields embedded in other types to be initialized to -1 as expectedFull diff of codegen output - extra eyes on this appreciated, as I haven't verified that all of the changes are reasonable: