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.

Sign up for GitHub

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

Remove member name from transparent associated constants #248

Merged
merged 4 commits into from
Jan 11, 2019
+203 −54
Merged

Remove member name from transparent associated constants #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a73020
Remove member name from transparent associated constants
grovesNL Nov 12, 2018
7c97da1
Handle enum associated constants and reduce memory usage
grovesNL Jan 8, 2019
3b37344
Run cargo-fmt
grovesNL Jan 8, 2019
c87c677
Remove unnecessary `clone`
grovesNL Jan 8, 2019
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
36 changes: 21 additions & 15 deletions src/bindgen/ir/constant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,37 +185,43 @@ impl Constant {
pub fn load_assoc(
name: String,
item: &syn::ImplItemConst,
impl_ty: &syn::Type,
mod_cfg: &Option<Cfg>,
is_transparent: bool,
struct_path: &Path,
) -> Result<Constant, String> {
let ty = Type::load(&item.ty)?;

if ty.is_none() {
return Err("Cannot have a zero sized const definition.".to_owned());
}
let ty = ty.unwrap();
if !ty.is_primitive_or_ptr_primitive()
&& match item.expr {
syn::Expr::Struct(_) => false,
_ => true,
}
{
return Err("Unhanded const definition".to_owned());
}

let impl_ty = Type::load(impl_ty)?;
if impl_ty.is_none() {
return Err("impl has an empty type".to_owned());
let can_handle_const_expr = match item.expr {
syn::Expr::Struct(_) => true,
_ => false,
};

if !ty.is_primitive_or_ptr_primitive() && !can_handle_const_expr {
return Err("Unhandled const definition".to_owned());
}
let impl_ty = impl_ty.unwrap();

let struct_path = impl_ty.get_root_path().unwrap();
let expr = Literal::load(match item.expr {
syn::Expr::Struct(syn::ExprStruct { ref fields, .. }) => {
if is_transparent && fields.len() == 1 {
&fields[0].expr
} else {
&item.expr
}
}
_ => &item.expr,
})?;

let full_name = Path::new(format!("{}_{}", struct_path, name));

Ok(Constant::new(
full_name,
ty,
Literal::load(&item.expr)?,
expr,
Cfg::append(mod_cfg, Cfg::load(&item.attrs)),
AnnotationSet::load(&item.attrs)?,
Documentation::load(&item.attrs),
Expand Down
131 changes: 100 additions & 31 deletions src/bindgen/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ use syn;
use bindgen::cargo::{Cargo, PackageRef};
use bindgen::error::Error;
use bindgen::ir::{
AnnotationSet, Cfg, Constant, Documentation, Enum, Function, GenericParams, ItemMap,
OpaqueItem, Path, Static, Struct, Typedef, Union,
AnnotationSet, Cfg, Constant, Documentation, Enum, Function, GenericParams, ItemContainer,
ItemMap, OpaqueItem, Path, Static, Struct, Type, Typedef, Union,
};
use bindgen::utilities::{SynAbiHelpers, SynItemHelpers};

Expand Down Expand Up @@ -490,6 +490,8 @@ impl Parse {
mod_cfg: &Option<Cfg>,
items: &[syn::Item],
) {
let mut impls_with_assoc_consts = Vec::new();

for item in items {
if item.has_test_attr() {
continue;
Expand Down Expand Up @@ -520,21 +522,31 @@ impl Parse {
self.load_syn_ty(crate_name, mod_cfg, item);
}
syn::Item::Impl(ref item_impl) => {
for item in &item_impl.items {
if let syn::ImplItem::Const(ref item) = item {
self.load_syn_assoc_const(
binding_crate_name,
crate_name,
mod_cfg,
&item_impl.self_ty,
item,
);
}
let has_assoc_const = item_impl.items.iter().any(|item| match item {
syn::ImplItem::Const(_) => true,
_ => false,
});
if has_assoc_const {
impls_with_assoc_consts.push(item_impl);
}
}
_ => {}
}
}

for item_impl in impls_with_assoc_consts {
let associated_constants = item_impl.items.iter().filter_map(|item| match item {
syn::ImplItem::Const(ref associated_constant) => Some(associated_constant),
_ => None,
});
self.load_syn_assoc_consts(
binding_crate_name,
crate_name,
mod_cfg,
&item_impl.self_ty,
associated_constants,
);
}
}

/// Enters a `extern "C" { }` declaration and loads function declarations.
Expand Down Expand Up @@ -632,36 +644,93 @@ impl Parse {
}
}

/// Loads an associated `const` declaration
fn load_syn_assoc_const(
fn is_assoc_const_of_transparent_struct(
&self,
const_item: &syn::ImplItemConst,
) -> Result<bool, ()> {
let ty = match Type::load(&const_item.ty) {
Ok(Some(t)) => t,
_ => return Ok(false),
};
let path = match ty.get_root_path() {
Some(p) => p,
_ => return Ok(false),
};

match self.structs.get_items(&path) {
Some(items) => {
if items.len() != 1 {
error!(
"Expected one struct to match path {}, but found {}",
path,
items.len(),
);
return Err(());
}

Ok(if let ItemContainer::Struct(ref s) = items[0] {
s.is_transparent
} else {
false
})
}
_ => Ok(false),
}
}

/// Loads associated `const` declarations
fn load_syn_assoc_consts<'a, I>(
&mut self,
binding_crate_name: &str,
crate_name: &str,
mod_cfg: &Option<Cfg>,
impl_ty: &syn::Type,
item: &syn::ImplItemConst,
) {
if crate_name != binding_crate_name {
info!(
"Skip {}::{} - (const's outside of the binding crate are not used).",
crate_name, &item.ident
);
items: I,
) where
I: IntoIterator<Item = &'a syn::ImplItemConst>,
{
let ty = Type::load(&impl_ty).unwrap();
if ty.is_none() {
return;
}

let const_name = item.ident.to_string();
let impl_path = ty.unwrap().get_root_path().unwrap();

match Constant::load_assoc(const_name.clone(), item, impl_ty, mod_cfg) {
Ok(constant) => {
info!("Take {}::{}.", crate_name, &item.ident);
for item in items.into_iter() {
let is_assoc_const_of_transparent_struct =
match self.is_assoc_const_of_transparent_struct(&item) {
Ok(b) => b,
Err(_) => continue,
};

let full_name = constant.path.clone();
if !self.constants.try_insert(constant) {
error!("Conflicting name for constant {}", full_name);
}
if crate_name != binding_crate_name {
info!(
"Skip {}::{} - (const's outside of the binding crate are not used).",
crate_name, &item.ident
);
continue;
}
Err(msg) => {
warn!("Skip {}::{} - ({})", crate_name, &item.ident, msg);

let const_name = item.ident.to_string();

match Constant::load_assoc(
const_name,
item,
mod_cfg,
is_assoc_const_of_transparent_struct,
&impl_path,
) {
Ok(constant) => {
info!("Take {}::{}.", crate_name, &item.ident);

let full_name = constant.path.clone();
if !self.constants.try_insert(constant) {
error!("Conflicting name for constant {}", full_name);
}
}
Err(msg) => {
warn!("Skip {}::{} - ({})", crate_name, &item.ident, msg);
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/expectations/both/const_conflict.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
#include <stdint.h>
#include <stdlib.h>

#define Foo_FOO 0
#define Foo_FOO 42
14 changes: 13 additions & 1 deletion tests/expectations/both/transparent.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

typedef struct DummyStruct DummyStruct;

typedef struct EnumWithAssociatedConstantInImpl EnumWithAssociatedConstantInImpl;

typedef DummyStruct TransparentComplexWrappingStructTuple;

typedef uint32_t TransparentPrimitiveWrappingStructTuple;
Expand All @@ -17,9 +19,19 @@ typedef DummyStruct TransparentComplexWrapper_i32;

typedef uint32_t TransparentPrimitiveWrapper_i32;

typedef uint32_t TransparentPrimitiveWithAssociatedConstants;

#define EnumWithAssociatedConstantInImpl_TEN 10

#define TransparentPrimitiveWithAssociatedConstants_ONE 1

#define TransparentPrimitiveWithAssociatedConstants_ZERO 0

void root(TransparentComplexWrappingStructTuple a,
TransparentPrimitiveWrappingStructTuple b,
TransparentComplexWrappingStructure c,
TransparentPrimitiveWrappingStructure d,
TransparentComplexWrapper_i32 e,
TransparentPrimitiveWrapper_i32 f);
TransparentPrimitiveWrapper_i32 f,
TransparentPrimitiveWithAssociatedConstants g,
EnumWithAssociatedConstantInImpl h);
2 changes: 1 addition & 1 deletion tests/expectations/const_conflict.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
#include <stdint.h>
#include <stdlib.h>

#define Foo_FOO 0
#define Foo_FOO 42
2 changes: 1 addition & 1 deletion tests/expectations/const_conflict.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
#include <cstdint>
#include <cstdlib>

static const int32_t Foo_FOO = 0;
static const uint32_t Foo_FOO = 42;
2 changes: 1 addition & 1 deletion tests/expectations/tag/const_conflict.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
#include <stdint.h>
#include <stdlib.h>

#define Foo_FOO 0
#define Foo_FOO 42
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change okay?

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'd actually prefer to fail in this case and not try to assume which is better to resolve the conflict, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds reasonable to me. I was just checking to see if the test expectation change was expected.

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 left it for now unless it's preferable to early exit in this case. The current behavior is to emit an error (with error!) and continue.

14 changes: 13 additions & 1 deletion tests/expectations/tag/transparent.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

struct DummyStruct;

struct EnumWithAssociatedConstantInImpl;

typedef struct DummyStruct TransparentComplexWrappingStructTuple;

typedef uint32_t TransparentPrimitiveWrappingStructTuple;
Expand All @@ -17,9 +19,19 @@ typedef struct DummyStruct TransparentComplexWrapper_i32;

typedef uint32_t TransparentPrimitiveWrapper_i32;

typedef uint32_t TransparentPrimitiveWithAssociatedConstants;

#define EnumWithAssociatedConstantInImpl_TEN 10

#define TransparentPrimitiveWithAssociatedConstants_ONE 1

#define TransparentPrimitiveWithAssociatedConstants_ZERO 0

void root(TransparentComplexWrappingStructTuple a,
TransparentPrimitiveWrappingStructTuple b,
TransparentComplexWrappingStructure c,
TransparentPrimitiveWrappingStructure d,
TransparentComplexWrapper_i32 e,
TransparentPrimitiveWrapper_i32 f);
TransparentPrimitiveWrapper_i32 f,
TransparentPrimitiveWithAssociatedConstants g,
struct EnumWithAssociatedConstantInImpl h);
14 changes: 13 additions & 1 deletion tests/expectations/transparent.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

typedef struct DummyStruct DummyStruct;

typedef struct EnumWithAssociatedConstantInImpl EnumWithAssociatedConstantInImpl;

typedef DummyStruct TransparentComplexWrappingStructTuple;

typedef uint32_t TransparentPrimitiveWrappingStructTuple;
Expand All @@ -17,9 +19,19 @@ typedef DummyStruct TransparentComplexWrapper_i32;

typedef uint32_t TransparentPrimitiveWrapper_i32;

typedef uint32_t TransparentPrimitiveWithAssociatedConstants;

#define EnumWithAssociatedConstantInImpl_TEN 10

#define TransparentPrimitiveWithAssociatedConstants_ONE 1

#define TransparentPrimitiveWithAssociatedConstants_ZERO 0

void root(TransparentComplexWrappingStructTuple a,
TransparentPrimitiveWrappingStructTuple b,
TransparentComplexWrappingStructure c,
TransparentPrimitiveWrappingStructure d,
TransparentComplexWrapper_i32 e,
TransparentPrimitiveWrapper_i32 f);
TransparentPrimitiveWrapper_i32 f,
TransparentPrimitiveWithAssociatedConstants g,
EnumWithAssociatedConstantInImpl h);
14 changes: 13 additions & 1 deletion tests/expectations/transparent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

struct DummyStruct;

struct EnumWithAssociatedConstantInImpl;

using TransparentComplexWrappingStructTuple = DummyStruct;

using TransparentPrimitiveWrappingStructTuple = uint32_t;
Expand All @@ -18,13 +20,23 @@ using TransparentComplexWrapper = DummyStruct;
template<typename T>
using TransparentPrimitiveWrapper = uint32_t;

using TransparentPrimitiveWithAssociatedConstants = uint32_t;

static const TransparentPrimitiveWrappingStructure EnumWithAssociatedConstantInImpl_TEN = 10;

static const TransparentPrimitiveWithAssociatedConstants TransparentPrimitiveWithAssociatedConstants_ONE = 1;

static const TransparentPrimitiveWithAssociatedConstants TransparentPrimitiveWithAssociatedConstants_ZERO = 0;

extern "C" {

void root(TransparentComplexWrappingStructTuple a,
TransparentPrimitiveWrappingStructTuple b,
TransparentComplexWrappingStructure c,
TransparentPrimitiveWrappingStructure d,
TransparentComplexWrapper<int32_t> e,
TransparentPrimitiveWrapper<int32_t> f);
TransparentPrimitiveWrapper<int32_t> f,
TransparentPrimitiveWithAssociatedConstants g,
EnumWithAssociatedConstantInImpl h);

} // extern "C"
Loading