Make HaspMap and RawTable to be repr(C)

[kvark]

Adding repr(C) isn't expected to change anything, since the RawTable consists purely of pointers and pointer-sized integers. It will, however, make it possible to pass hashmaps across the FFI boundary, which would be useful to Gecko-integrated Rust projects, such as wgpu.
cc @Gankra

[Gankra]

I think this should be ~fine to do, as we aren't guaranteeing these facts in the standard library, just in HashBrown, which is ostensibly dedicated to being a particular implementation. Furthermore the cbindgen usecase doesn't preclude actually changing the layout of these structs, because all that usecase needs is the ability to compute the current layout; not a guarantee that future versions won't change the layout.

Specifically outlining/inling/removing/adding fields is fine. More specifically, adding an Allocator generic parameter/field is also fine, because the default Global one should be zero-sized, and cbindgen ignores zero-sized things (just as it would for BuildHasherDefault).

[Gankra]

Clarification: adding an FFI-hostile item to the representation would be "breaking". So anything non-trivial from std, or almost any tagged union besides Option<NonNull>. I do not anticipate this needing to be done.

[Amanieu]

AFAIK you don't need repr(C) to pass a pointer to a HashMap through the FFI boundary. repr(C) is only needed if you pass a HashMap by value, but then the FFI side would need to know the exact size and alignment of the HashMap object.

[kvark]

@Amanieu I do want to pass it by value in order to avoid indirection in hot code.

[Amanieu]

I don't think repr(C) will really help here since all it does is fix the field order in the struct. Generally the internal details of the HashMap type, including its fields and their ordering, is not something that we provide API guarantees for.

Your best bet here might be to have an assert to check that the HashMap size/align are what you expect them to be, and then silence the FFI-unsafe warning.

Could you show me the code where you try to pass a HashMap through FFI?

[Gankra]

we don't need size/align to be guaranteed, only for it to be derivable from the source code (using cbindgen to generate a header at build time). without repr(c) this cannot be done reliably as rust makes no guarantees. furthermore, field ordering/types are required for pass-by-value, because ABIs scalarize the fields of structs into registers based on their types.

…

On Mon, Jan 13, 2020 at 2:51 PM Amanieu ***@***.***> wrote: I don't think repr(C) will really help here since all it does is fix the field order in the struct. Generally the internal details of the HashMap type, including its fields and their ordering, is not something that we provide API guarantees for. Your best bet here might be to have an assert to check that the HashMap size/align are what you expect them to be, and then silence the FFI-unsafe warning. Could you show me the code where you try to pass a HashMap through FFI? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#140?email_source=notifications&email_token=AAIVRYGH6LUAECUBW56NFN3Q5TA4VA5CNFSM4KGGJHXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI2BVFY#issuecomment-573840023>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIVRYBFFU7X7VLEQ3KM6NTQ5TA4VANCNFSM4KGGJHXA> .

[Amanieu]

Can you add a comment explaining exactly why we are adding repr(C) here?

[Amanieu]

And here.

[Gankra]

Oh right yea I was discussing this more with kvark and reminded myself of The Destructor Problem. Destructors affect ABI significantly, so in reality HashMap shouldn't ever be passed by-value, it should be passed by-mutable-ref and swapped. That said it's still "fine" to store in C++ as long as you're fine with leaks.

[bors]

☔ The latest upstream changes (presumably #146) made this pull request unmergeable. Please resolve the merge conflicts.

[Gankra]

we should just close this