Skip to content

Use placement new for constructing in tagged unions' helper methods.#336

Merged
emilio merged 1 commit intomozilla:masterfrom
emilio:place-new
May 10, 2019
Merged

Use placement new for constructing in tagged unions' helper methods.#336
emilio merged 1 commit intomozilla:masterfrom
emilio:place-new

Conversation

@emilio
Copy link
Copy Markdown
Collaborator

@emilio emilio commented May 7, 2019

Using operator= is not quite sound in presence of destructors and operator
overloading.

It's perfectly fine to assume that the left-hand-side of an operator= expression
is valid memory, however we're using uninitialized memory here, that may not be
the case.

Use placement new to properly construct tagged unions. I don't need this with
any urgency, but it's the right thing to do in presence of complex types, and
the current code seems a bomb waiting to explode :)

@emilio
Copy link
Copy Markdown
Collaborator Author

emilio commented May 7, 2019

r? @eqrion or @gankro

Copy link
Copy Markdown

@mystor mystor left a comment

Choose a reason for hiding this comment

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

@gankro asked me to pop in & add some comments which I added from over IRC :-)

Using operator= is not quite sound in presence of destructors and operator
overloading.

It's perfectly fine to assume that the left-hand-side of an operator= expression
is valid memory, however we're using uninitialized memory here, that may not be
the case.

Use placement new to properly construct tagged unions. I don't need this with
any urgency, but it's the right thing to do in presence of complex types, and
the current code seems a bomb waiting to explode :)
@emilio emilio merged commit e999d32 into mozilla:master May 10, 2019
@emilio emilio deleted the place-new branch May 10, 2019 16:34
@emilio emilio restored the place-new branch May 10, 2019 16:34
@emilio emilio deleted the place-new branch May 10, 2019 16:34
@emilio
Copy link
Copy Markdown
Collaborator Author

emilio commented May 10, 2019

18:03 <emilio> I addressed your comments in https://github.com/eqrion/cbindgen/pull/336, and will apply the ::new() suggestion over in the other PR
18:03 <emilio> I assume it looks good to you with that?
18:06 <nika> Yeah, looks good.

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.

2 participants