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

Use original name when checking allowlist for anonymous enum variants #2006

Merged
merged 2 commits into from Mar 22, 2021

Conversation

jethrogb
Copy link
Contributor

@jethrogb jethrogb commented Mar 10, 2021

Prior to this PR, this header

enum {
    MyVal = 0,
};

in combination with allowlist_var("^MyVal$") and an implementation of ParseCallbacks::enum_variant_name would generate no code. This is because the renamed variant name is checked against the variable allowlist. All other allowlist checks are against the unrenamed name. This PR makes that consistent. This is a breaking change but I think the old behavior is clearly wrong.

Additionally, I've added a way to test ParseCallbacks in CI.

)]

pub const RENAMED_MyVal: ::std::os::raw::c_uint = 0;
pub type _bindgen_ty_1 = ::std::os::raw::c_uint;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit confused as to why this type is generated but not used in the definition of RENAMED_MyVal.

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

So this looks sensible, but regarding testing: The way we've been testing these has been using the bindgen-integration crate. It seems it'd be easy to add a test for this there rather than adding new testing infrastructure, wdyt?

@jethrogb
Copy link
Contributor Author

regarding testing

I see, I didn't realize that existed. Looking at it briefly, the bindgen-integration is quite monolithic and it doesn't check the exact output. I think the expectation testing framework is much better, so I'd prefer to do it this way. But if you feel strongly about it I can move the tests to bindgen-integration.

@emilio
Copy link
Contributor

emilio commented Mar 14, 2021

regarding testing

I see, I didn't realize that existed. Looking at it briefly, the bindgen-integration is quite monolithic and it doesn't check the exact output. I think the expectation testing framework is much better, so I'd prefer to do it this way. But if you feel strongly about it I can move the tests to bindgen-integration.

Ok, I'm fine with that, it's not too complicated anyways. Thanks!

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Just one question, but looks fine otherwise.

tests/tests.rs Outdated Show resolved Hide resolved
tests/tests.rs Outdated Show resolved Hide resolved
@jethrogb jethrogb force-pushed the jb/anonymous-enum-variant-rename branch from 7f45e2d to 2a546c8 Compare March 16, 2021 10:17
@emilio emilio merged commit 2a46e29 into rust-lang:master Mar 22, 2021
bors bot added a commit to fortanix/rust-mbedtls that referenced this pull request Apr 6, 2021
152: Update bindgen r=raoulstrackx a=jethrogb

I took some time this week to make the necessary bindgen changes (rust-lang/rust-bindgen#2004 rust-lang/rust-bindgen#2006 rust-lang/rust-bindgen#2007). This PR updates the bindgen build to use that.

Breaking changes:
* unnamed types are renamed
* C-unions are now actual unions
  * [x] the field accessor functions that used to exist are easy enough to add back
* bitfields are done differently now
* some fn-ptrs are now unsafe
* some fns and fn-ptrs now take const pointers as arguments
* havege.c and timing.c are now not compiled on non-unix platforms, i.e. SGX (probably wasn't working properly anyway)

Fixes #5 #14 #61 #72 #88 #121

Co-authored-by: Jethro Beekman <jethro@fortanix.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants