-
Notifications
You must be signed in to change notification settings - Fork 715
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
Feature 699 constified enum module #741
Changes from 6 commits
5762339
0826846
fb9959a
977ec6f
de155b9
5f4b730
4b10529
465e242
814d28e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
//! Bindgen's core intermediate representation type. | ||
|
||
use super::super::codegen::CONSTIFIED_ENUM_MODULE_REPR_NAME; | ||
use super::annotations::Annotations; | ||
use super::context::{BindgenContext, ItemId, PartialType}; | ||
use super::derive::{CanDeriveCopy, CanDeriveDebug, CanDeriveDefault}; | ||
|
@@ -825,6 +826,19 @@ impl Item { | |
_ => None, | ||
} | ||
} | ||
|
||
/// Returns whether the item is a constified module enum | ||
fn is_constified_enum_module(&self, ctx: &BindgenContext) -> bool { | ||
if let ItemKind::Type(ref type_) = self.kind { | ||
if let Some(ref type_) = type_.safe_canonical_type(ctx) { | ||
if let TypeKind::Enum(ref enum_) = *type_.kind() { | ||
return enum_.is_constified_enum_module(ctx, self); | ||
} | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
} | ||
|
||
/// A set of items. | ||
|
@@ -1443,10 +1457,18 @@ impl ItemCanonicalPath for Item { | |
fn namespace_aware_canonical_path(&self, | ||
ctx: &BindgenContext) | ||
-> Vec<String> { | ||
let path = self.canonical_path(ctx); | ||
let mut path = self.canonical_path(ctx); | ||
let is_constified_module_enum = self.is_constified_enum_module(ctx); | ||
if ctx.options().enable_cxx_namespaces { | ||
if is_constified_module_enum { | ||
path.push(CONSTIFIED_ENUM_MODULE_REPR_NAME.into()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the check should be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I guess, but that relies on |
||
} | ||
return path; | ||
} | ||
if is_constified_module_enum { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To fix the last test-case I commented about, you should remove this early return and let it arrive to the last return. For that to work, you need to add the if is_constified_module_enum {
path.push(CONSTIFIED_ENUM_MODULE_REPR_NAME.into());
}
if ctx.options().enable_cxx_namespaces {
return path;
}
if ctx.options().disable_name_namespacing {
let trailing_segments = if is_constified_module_enum { 2 } else { 1 };
return path[..path.len() - trailing_segments].iter().cloned().collect();
}
return vec![path[1..].join("_")]; |
||
return vec![path.last().unwrap().clone(), | ||
CONSTIFIED_ENUM_MODULE_REPR_NAME.into()]; | ||
} | ||
if ctx.options().disable_name_namespacing { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this branch is correct. |
||
return vec![path.last().unwrap().clone()]; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/* automatically generated by rust-bindgen */ | ||
|
||
|
||
#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)] | ||
|
||
|
||
pub mod foo { | ||
pub type Type = ::std::os::raw::c_uint; | ||
pub const THIS: Type = 0; | ||
pub const SHOULD_BE: Type = 1; | ||
pub const A_CONSTANT: Type = 2; | ||
} | ||
pub use self::foo::Type as foo_alias1; | ||
pub use self::foo_alias1 as foo_alias2; | ||
#[repr(C)] | ||
#[derive(Debug, Copy)] | ||
pub struct bar { | ||
pub this_should_work: foo::Type, | ||
} | ||
#[test] | ||
fn bindgen_test_layout_bar() { | ||
assert_eq!(::std::mem::size_of::<bar>() , 4usize , concat ! ( | ||
"Size of: " , stringify ! ( bar ) )); | ||
assert_eq! (::std::mem::align_of::<bar>() , 4usize , concat ! ( | ||
"Alignment of " , stringify ! ( bar ) )); | ||
assert_eq! (unsafe { | ||
& ( * ( 0 as * const bar ) ) . this_should_work as * const _ | ||
as usize } , 0usize , concat ! ( | ||
"Alignment of field: " , stringify ! ( bar ) , "::" , | ||
stringify ! ( this_should_work ) )); | ||
} | ||
impl Clone for bar { | ||
fn clone(&self) -> Self { *self } | ||
} | ||
impl Default for bar { | ||
fn default() -> Self { unsafe { ::std::mem::zeroed() } } | ||
} | ||
extern "C" { | ||
pub fn func1(arg1: foo::Type, arg2: *mut foo::Type, | ||
arg3: *mut *mut foo::Type) -> *mut foo::Type; | ||
} | ||
extern "C" { | ||
pub fn func2(arg1: foo_alias1, arg2: *mut foo_alias1, | ||
arg3: *mut *mut foo_alias1) -> *mut foo_alias1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what you're trying to do here is resolving through
ResolvedTypeRef
s, but not throughAlias
, right?If so, instead of reinventing the wheel, you can do something like:
And keep the comment on why not through type alias.
We should arguably unify that and the
canonical_type
business, but that also handles some template stuff that may be nontrivial, so it's worth doing on a followup.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check needs to "peel back" one layer of
Alias
es. The issue with theresolve()
function is that it resolves through all of theAlias
es.I think that implementation should remain as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I see. I guess I'm not sure why would making an alias claim it's a constified enum module be necessary.
I guess I'll poke a bit at the code this evening when I have the chance and see which tests break with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... Ok, so what breaks in that case is the
typedef enum
case... fun. I still think we should be able to do better, though...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about modifying
ItemResolver
to optionally disable recursive resolving? More generally,ItemResolver
could even take a positive integer indicating through how many layers to resolve.This way,
is_constified_enum_module()
could be simplified to useItemResolver
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is about the number of layers to resolve. I think the logic in
is_constified_enum_module
is wrong, and the "alias to resolved type ref to alias" logic is just masking it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I came up with, I think this is what you're really trying to do with that function:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to address the
TODO
if you want, btw :)