Skip to content

Add support for 'unsafe fields' #20

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

Merged
merged 3 commits into from
Jul 26, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ script:
- cargo build --verbose --features llvm_stable
- make test
- git add -A
- git diff @
- git diff-index --quiet HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do the same removing the --quiet here

Copy link
Member Author

Choose a reason for hiding this comment

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

No, git diff-index only gives an error on empty diffs if run with --quiet.

(It also outputs object ids, not diffs)


notifications:
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ BINDGEN := ./target/debug/bindgen

.PHONY: $(BINDGEN)
$(BINDGEN):
[ -f $@ ] || cargo build
[ -f $@ ] || cargo build --features llvm_stable

.PHONY: test
test: regen-tests
Expand Down
67 changes: 61 additions & 6 deletions src/gen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use syntax::print::pprust::tts_to_string;

use super::BindgenOptions;
use super::LinkType;
use parser::Accessor;
use types::*;
use aster;

Expand Down Expand Up @@ -840,6 +841,52 @@ fn comp_attrs(ctx: &GenCtx, ci: &CompInfo, name: &str, extra: &mut Vec<P<ast::It
attrs
}

fn gen_accessors(ctx: &mut GenCtx, name: &str, ty: &ast::Ty, accessor: Accessor,
methods: &mut Vec<ast::ImplItem>) {
if accessor == Accessor::None {
return;
}
let ident = ctx.ext_cx.ident_of(&format!("{}", name));
let mutable_getter_name = ctx.ext_cx.ident_of(&format!("get_{}_mut", name));
let getter_name = ctx.ext_cx.ident_of(&format!("get_{}", name));
let imp = match accessor {
Accessor::Regular => quote_item!(&ctx.ext_cx,
impl X {
#[inline]
pub fn $getter_name(&self) -> & $ty {
& self.$ident
}
pub fn $mutable_getter_name(&mut self) -> &mut $ty {
&mut self.$ident
}
}
),
Accessor::Unsafe => quote_item!(&ctx.ext_cx,
impl X {
#[inline]
pub unsafe fn $getter_name(&self) -> & $ty {
& self.$ident
}
pub unsafe fn $mutable_getter_name(&mut self) -> &mut $ty {
&mut self.$ident
}
}
),
Accessor::Immutable => quote_item!(&ctx.ext_cx,
impl X {
#[inline]
pub fn $getter_name(&self) -> & $ty {
& self.$ident
}
}
),
_ => return
};
match imp.unwrap().node {
ast::ItemKind::Impl(_, _, _, _, _, ref items) => methods.extend(items.clone()),
_ => unreachable!()
}
}

fn cstruct_to_rs(ctx: &mut GenCtx, name: &str, ci: CompInfo) -> Vec<P<ast::Item>> {
let layout = ci.layout;
Expand Down Expand Up @@ -878,14 +925,15 @@ fn cstruct_to_rs(ctx: &mut GenCtx, name: &str, ci: CompInfo) -> Vec<P<ast::Item>
};

if let Some(ref base) = base_vftable {
vffields.push(ast::StructField {
let field = ast::StructField {
span: ctx.span,
vis: ast::Visibility::Public,
ident: Some(ctx.ext_cx.ident_of("_base")),
id: ast::DUMMY_NODE_ID,
ty: P(mk_ty(ctx, false, &[base.clone()])),
attrs: vec![],
});
};
vffields.push(field);
}

for vm in ci.vmethods.iter() {
Expand Down Expand Up @@ -1052,15 +1100,22 @@ fn cstruct_to_rs(ctx: &mut GenCtx, name: &str, ci: CompInfo) -> Vec<P<ast::Item>
} else {
rust_ty
};

fields.push(ast::StructField {
let vis = if f.private {
ast::Visibility::Inherited
} else {
ast::Visibility::Public
};
gen_accessors(ctx, &f_name, &rust_ty, f.accessor, &mut methods);
let field = ast::StructField {
span: ctx.span,
ident: Some(ctx.ext_cx.ident_of(&f_name)),
vis: ast::Visibility::Public,
vis: vis,
id: ast::DUMMY_NODE_ID,
ty: rust_ty,
attrs: mk_doc_attr(ctx, &f.comment)
});
};
fields.push(field);

if bypass {
continue;
}
Expand Down
39 changes: 36 additions & 3 deletions src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ fn decl_name(ctx: &mut ClangParserCtx, cursor: &Cursor) -> Global {
CXCursor_ClassTemplate |
CXCursor_ClassDecl |
CXCursor_StructDecl => {
let anno = Annotations::new(&cursor);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... This is for nested structs right? Can you add a test for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is for annotating the entire struct with something. See the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I'm pretty sure we parse that at another place. huh. I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's visit_top, which AFAICT is only called for toplevel decls. decl_name otoh is called for all decls

Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing visit_top shouldn't visit (for a class/struct/union decl), are nested structs, when we directly call visit_composite IIRC.

So I'm fine with moving it here, but there's no point in parsing it twice, that's what I wanted to say. Nontheless, I think that would bring a bit of churn here right? I'd be satisfied if you file an issue so I can refactor that when I have the time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #26

let kind = if cursor.kind() == CXCursor_UnionDecl {
CompKind::Union
} else {
Expand Down Expand Up @@ -159,7 +160,7 @@ fn decl_name(ctx: &mut ClangParserCtx, cursor: &Cursor) -> Global {
}
};

let mut ci = CompInfo::new(spelling, ctx.current_module_id, filename, comment, kind, vec![], layout);
let mut ci = CompInfo::new(spelling, ctx.current_module_id, filename, comment, kind, vec![], layout, anno);
ci.parser_cursor = Some(cursor);

// If it's an instantiation of another template,
Expand Down Expand Up @@ -538,12 +539,34 @@ fn opaque_ty(ctx: &mut ClangParserCtx, ty: &cx::Type) {
}
}

struct Annotations {
#[derive(Copy, PartialEq, Clone, Debug)]
pub enum Accessor {
None,
Regular,
Unsafe,
Immutable,
}

#[derive(Clone, PartialEq, Debug)]
pub struct Annotations {
opaque: bool,
hide: bool,
use_as: Option<String>,
/// Disable deriving copy/clone on this struct.
no_copy: bool,
// In the None case we fall back to the value specified
// in the enclosing decl
private: Option<bool>,
accessor: Option<Accessor>,
}

fn parse_accessor(s: &str) -> Accessor {
match s {
"false" => Accessor::None,
"unsafe" => Accessor::Unsafe,
"immutable" => Accessor::Immutable,
_ => Accessor::Regular,
}
}

impl Annotations {
Expand All @@ -553,6 +576,8 @@ impl Annotations {
hide: false,
use_as: None,
no_copy: false,
private: None,
accessor: None
};

anno.parse(&cursor.comment());
Expand All @@ -571,6 +596,10 @@ impl Annotations {
"hide" => self.hide = true,
"replaces" => self.use_as = Some(comment.get_tag_attr_value(i)),
"nocopy" => self.no_copy = true,
"private" => self.private = Some(comment.get_tag_attr_value(i) != "false"),
"accessor" => {
self.accessor = Some(parse_accessor(&comment.get_tag_attr_value(i)))
},
_ => (),
}
}
Expand Down Expand Up @@ -792,12 +821,16 @@ fn visit_composite(cursor: &Cursor, parent: &Cursor,

if should_replace {
*info = FieldInfo::new(name, ty, comment, bitfields, mutable);
info.private = anno.private.unwrap_or(ci.anno.private.unwrap_or(false));
info.accessor = anno.accessor.unwrap_or(ci.anno.accessor.unwrap_or(Accessor::None));
return CXChildVisit_Continue;
}
}
}

let field = FieldInfo::new(name, ty, comment, bitfields, mutable);
let mut field = FieldInfo::new(name, ty, comment, bitfields, mutable);
field.private = anno.private.unwrap_or(ci.anno.private.unwrap_or(false));
field.accessor = anno.accessor.unwrap_or(ci.anno.accessor.unwrap_or(Accessor::None));
ci.members.push(CompMember::Field(field));
}
CXCursor_StructDecl |
Expand Down
17 changes: 16 additions & 1 deletion src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ pub use self::IKind::*;
pub use self::FKind::*;
use clang::Cursor;

use parser::{Annotations, Accessor};

static NEXT_MODULE_ID: AtomicUsize = ATOMIC_USIZE_INIT;

#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug)]
Expand Down Expand Up @@ -446,6 +448,9 @@ pub struct CompInfo {
/// Used to detect if we've run in a has_destructor cycle while cycling
/// around the template arguments.
detect_has_destructor_cycle: Cell<bool>,

/// Annotations on the decl
pub anno: Annotations,
}

static mut UNNAMED_COUNTER: u32 = 0;
Expand All @@ -466,7 +471,8 @@ impl CompInfo {
comment: String,
kind: CompKind,
members: Vec<CompMember>,
layout: Layout) -> CompInfo {
layout: Layout,
anno: Annotations) -> CompInfo {
let was_unnamed = name.is_empty();
CompInfo {
kind: kind,
Expand Down Expand Up @@ -494,6 +500,7 @@ impl CompInfo {
was_unnamed: was_unnamed,
detect_derive_debug_cycle: Cell::new(false),
detect_has_destructor_cycle: Cell::new(false),
anno: anno,
}
}

Expand Down Expand Up @@ -640,6 +647,12 @@ pub struct FieldInfo {
pub bitfields: Option<Vec<(String, u32)>>,
/// If the C++ field is marked as `mutable`
pub mutable: bool,
/// True when field or enclosing struct
/// has a `<div rust-bindgen private>` annotation
pub private: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a doc comment here saying when this can be true (right now only explicitly via annotations right?)

/// Set by the `<div rust-bindgen accessor="..">`
/// annotation on a field or enclosing struct
pub accessor: Accessor,
}

impl FieldInfo {
Expand All @@ -654,6 +667,8 @@ impl FieldInfo {
comment: comment,
bitfields: bitfields,
mutable: mutable,
private: false,
accessor: Accessor::None,
}
}
}
Expand Down
Loading