Skip to content

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Mar 25, 2020

Do idiomatic C++ copy and move constructors for a few things, so
wrapping structs' defaults can work.

@Ericson2314
Copy link
Member Author

Also serves as bisect for #3439

Do idiomatic C++ copy and move constructors for a few things, so
wrapping structs' defaults can work.

void operator = (const StorePath & that)
{
(rust::Value<3 * sizeof(void *) + 24, ffi_StorePath_drop>::operator = (that));
Copy link
Member

Choose a reason for hiding this comment

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

Where does that magic size come from? I see that we start to duplicate that throughout (at least) this file a few times. Shouldn't there be a comment where the magic 24 comes from? For the 3 * sizeof(void *) part one can probably guess that the struct hold 3 pointers. The 24 bytes could be 3 integers? Can't we use a single sizeof(SomeStruct) instead? Does this take into account alignment changes if a new member is added to whatever the underlying struct is?

I know you didn't introduce it but this might go out of sync with the other definition thus I am commenting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind the numbers, I am repeating the whole superclass. Maybe I should #ifdef and then #undef the superclass; that would help without leaking anything more to the files the include the header.

@Ericson2314
Copy link
Member Author

@edolstra It has come up again in #3673 and #3523 that the lack of copy constructors this PR fixes prevents us from using idiomatic C++ STL functions, which is a bummer. Is there anything I can do to make this acceptable for you? For example, after #3671 is merged, I plan on writing additional unit tests to ensure clone is appropriately called.

@Ericson2314
Copy link
Member Author

To someday return to if a Rust rewrite happens.

@maisiliym
Copy link

@Ericson2314 Ericson2314 deleted the more-rust-ffi branch October 7, 2021 16:00
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.

5 participants