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

cbindgen 0.6.4 moved constants outside namespace #229

Closed
staktrace opened this issue Oct 19, 2018 · 6 comments
Closed

cbindgen 0.6.4 moved constants outside namespace #229

staktrace opened this issue Oct 19, 2018 · 6 comments

Comments

@staktrace
Copy link
Contributor

The constants at https://searchfox.org/mozilla-central/rev/9cb3e241502a2d47e2d5057ca771324a446b6695/gfx/webrender_bindings/webrender_ffi_generated.h#20,22 are inside the mozilla::wr namespace. With cbindgen 0.6.4 they get moved outside the namespace which is undesirable.

@staktrace
Copy link
Contributor Author

Actually these constants are generated from https://searchfox.org/mozilla-central/rev/9cb3e241502a2d47e2d5057ca771324a446b6695/gfx/webrender_bindings/src/program_cache.rs#16 and are never used on the C++ side so if there's a way to omit them from being generated that would be fine too.

@staktrace
Copy link
Contributor Author

This appears to be a result of the changes in #170 to bindings.rs, it explicitly moved the namespace opening down to after the constants were written. @IGI-111 was this intentional? None of the tests added seem to have namespaces so I'm wondering if this change was accidental.

@staktrace
Copy link
Contributor Author

I guess it was intentional, because only constants of type is_primitive_or_ptr_primitive() got moved outside the namespace. But I'm not sure why.

@eqrion
Copy link
Collaborator

eqrion commented Oct 19, 2018

I don't recall the reason why constants of type is_primitive_or_ptr_primitive() were moved outside namespaces. I know !is_primitive_or_ptr_primitive() were moved to be after all items were output so they have the right definitions.

@eqrion
Copy link
Collaborator

eqrion commented Oct 19, 2018

@staktrace As a stopgap you could skip emitting constants.

[export]
# don't have a negative version of item-types unfortunately
# item-types = ["Constants", "Globals", "Enums", "Structs", "Unions", "Typedefs", "OpaqueItems", "Functions"]
item-types = ["Globals", "Enums", "Structs", "Unions", "Typedefs", "OpaqueItems", "Functions"]

IGI-111 added a commit to IGI-111/cbindgen that referenced this issue Oct 22, 2018
Place constants back inside namespace and add a test to make sure they
are namespaced correctly.
@IGI-111
Copy link
Contributor

IGI-111 commented Oct 22, 2018

That wasn't an intentional change. I suspect either automerging lines that moved around or my own dumb self borking a conflicting merge.

In any case, there should probably be some tests for the namespacing feature, right now there doesn't seem to be any?

IGI-111 added a commit to IGI-111/cbindgen that referenced this issue Oct 22, 2018
Place constants back inside namespace and add a test to make sure they
are namespaced correctly.
IGI-111 added a commit to IGI-111/cbindgen that referenced this issue Oct 22, 2018
Place constants back inside namespace and add a test to make sure they
are namespaced correctly.
staktrace added a commit to staktrace/cbindgen that referenced this issue Oct 22, 2018
IGI-111 added a commit to IGI-111/cbindgen that referenced this issue Oct 22, 2018
Place constants back inside namespace and add a test to make sure they
are namespaced correctly.
@eqrion eqrion closed this as completed in d32127e Oct 24, 2018
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

No branches or pull requests

3 participants