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 duplicated field enumeration #1194

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Remove duplicated field enumeration #1194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
16c8a70
Add test for issue 1192.
adetaylor Nov 27, 2022
a7c4d35
Fix the test
adetaylor Nov 27, 2022
81a3d96
Log some more details of some APIs.
adetaylor Nov 27, 2022
bd0efed
Fix issue 1192.
adetaylor Nov 27, 2022
3819cfc
Remove duplicated field enumeration logic.
adetaylor Nov 27, 2022
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
22 changes: 15 additions & 7 deletions engine/src/conversion/analysis/abstract_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use super::{
FnAnalysis, FnKind, FnPhase, FnPrePhase2, MethodKind, PodAndConstructorAnalysis,
TraitMethodKind,
},
pod::PodAnalysis,
pod::{FieldsInfo, PodAnalysis},
};
use crate::conversion::{api::Api, apivec::ApiVec};
use crate::conversion::{
Expand Down Expand Up @@ -62,9 +62,13 @@ pub(crate) fn mark_types_abstract(mut apis: ApiVec<FnPrePhase2>) -> ApiVec<FnPre
bases,
kind: TypeKind::Pod | TypeKind::NonPod,
castable_bases,
field_deps,
field_definition_deps,
field_info,
fields:
FieldsInfo {
field_deps,
field_definition_deps,
field_info,
..
},
is_generic,
in_anonymous_namespace,
},
Expand All @@ -84,9 +88,13 @@ pub(crate) fn mark_types_abstract(mut apis: ApiVec<FnPrePhase2>) -> ApiVec<FnPre
bases,
kind: TypeKind::Abstract,
castable_bases,
field_deps,
field_definition_deps,
field_info,

fields: FieldsInfo {
field_deps,
field_definition_deps,
field_info,
field_conversion_errors: Vec::new(),
},
is_generic,
in_anonymous_namespace,
},
Expand Down
13 changes: 8 additions & 5 deletions engine/src/conversion/analysis/deps.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crate::{

use super::{
fun::{FnPhase, FnPrePhase1, PodAndDepAnalysis},
pod::PodAnalysis,
pod::{FieldsInfo, PodAnalysis},
tdef::TypedefAnalysis,
};

Expand All @@ -37,9 +37,12 @@ impl HasDependencies for Api<FnPrePhase1> {
..
} => Box::new(old_tyname.iter().chain(deps.iter())),
Api::Struct {
analysis: PodAnalysis {
bases, field_deps, ..
},
analysis:
PodAnalysis {
bases,
fields: FieldsInfo { field_deps, .. },
..
},
..
} => Box::new(field_deps.iter().chain(bases.iter())),
Api::Function { analysis, .. } => Box::new(analysis.deps.iter()),
Expand Down Expand Up @@ -74,7 +77,7 @@ impl HasDependencies for Api<FnPhase> {
PodAnalysis {
kind: TypeKind::Pod,
bases,
field_deps,
fields: FieldsInfo { field_deps, .. },
..
},
constructor_and_allocator_deps,
Expand Down
3 changes: 2 additions & 1 deletion engine/src/conversion/analysis/fun/implicit_constructors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use indexmap::{map::Entry, set::IndexSet as HashSet};

use syn::{Type, TypeArray};

use crate::conversion::analysis::pod::FieldsInfo;
use crate::conversion::api::DeletedOrDefaulted;
use crate::{
conversion::{
Expand Down Expand Up @@ -205,7 +206,7 @@ pub(super) fn find_constructors_present(
analysis:
PodAnalysis {
bases,
field_info,
fields: FieldsInfo { field_info, .. },
is_generic: false,
in_anonymous_namespace: false,
..
Expand Down
8 changes: 6 additions & 2 deletions engine/src/conversion/analysis/fun/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ use self::{
use super::{
depth_first::HasFieldsAndBases,
doc_label::make_doc_attrs,
pod::{PodAnalysis, PodPhase},
pod::{FieldsInfo, PodAnalysis, PodPhase},
tdef::TypedefAnalysis,
type_converter::{Annotated, PointerTreatment},
};
Expand Down Expand Up @@ -2238,7 +2238,11 @@ impl HasFieldsAndBases for Api<FnPrePhase1> {
Api::Struct {
analysis:
PodAnalysis {
field_definition_deps,
fields:
FieldsInfo {
field_definition_deps,
..
},
bases,
..
},
Expand Down
95 changes: 55 additions & 40 deletions engine/src/conversion/analysis/pod/byvalue_checker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,20 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use crate::conversion::analysis::depth_first::fields_and_bases_first;
use crate::conversion::apivec::ApiVec;
use crate::{conversion::ConvertErrorFromCpp, known_types::known_types};
use crate::{
conversion::{
analysis::tdef::TypedefPhase,
api::{Api, TypedefKind},
},
conversion::api::{Api, TypedefKind},
types::{Namespace, QualifiedName},
};
use crate::{conversion::ConvertErrorFromCpp, known_types::known_types};
use autocxx_parser::IncludeCppConfig;
use indexmap::IndexSet;
use std::collections::HashMap;
use syn::{ItemStruct, Type};

use super::FieldsDeterminedPhase;

#[derive(Clone)]
enum PodState {
UnsafeToBePod(String),
Expand All @@ -30,14 +31,14 @@ enum PodState {
#[derive(Clone)]
struct StructDetails {
state: PodState,
dependent_structs: Vec<QualifiedName>,
dependent_structs: IndexSet<QualifiedName>,
}

impl StructDetails {
fn new(state: PodState) -> Self {
StructDetails {
state,
dependent_structs: Vec::new(),
dependent_structs: IndexSet::new(),
}
}
}
Expand All @@ -48,6 +49,9 @@ impl StructDetails {
/// std::string contains a self-referential pointer.
/// It is possible that this is duplicative of the information stored
/// elsewhere in the `Api` list and could possibly be removed or simplified.
/// In general this is one of the oldest parts of autocxx and
/// the code here could quite possibly be simplified by reusing code
/// elsewhere.
pub struct ByValueChecker {
// Mapping from type name to whether it is safe to be POD
results: HashMap<QualifiedName, StructDetails>,
Expand All @@ -70,7 +74,7 @@ impl ByValueChecker {
/// Scan APIs to work out which are by-value safe. Constructs a [ByValueChecker]
/// that others can use to query the results.
pub(crate) fn new_from_apis(
apis: &ApiVec<TypedefPhase>,
apis: &ApiVec<FieldsDeterminedPhase>,
config: &IncludeCppConfig,
) -> Result<ByValueChecker, ConvertErrorFromCpp> {
let mut byvalue_checker = ByValueChecker::new();
Expand All @@ -81,7 +85,7 @@ impl ByValueChecker {
.results
.insert(tn, StructDetails::new(safety));
}
for api in apis.iter() {
for api in fields_and_bases_first(apis.iter()) {
match api {
Api::Typedef { analysis, .. } => {
let name = api.name();
Expand Down Expand Up @@ -113,19 +117,18 @@ impl ByValueChecker {
None => byvalue_checker.ingest_nonpod_type(name.clone()),
}
}
Api::Struct { details, .. } => {
byvalue_checker.ingest_struct(&details.item, api.name().get_namespace())
}
Api::Enum { .. } => {
byvalue_checker
.results
.insert(api.name().clone(), StructDetails::new(PodState::IsPod));
}
Api::ExternCppType { pod: true, .. } => {
Api::Enum { .. } | Api::ExternCppType { pod: true, .. } => {
byvalue_checker
.results
.insert(api.name().clone(), StructDetails::new(PodState::IsPod));
}
Api::Struct {
details, analysis, ..
} => byvalue_checker.ingest_struct(
&details.item,
api.name().get_namespace(),
&analysis.field_deps,
),
_ => {}
}
}
Expand All @@ -140,12 +143,16 @@ impl ByValueChecker {
Ok(byvalue_checker)
}

fn ingest_struct(&mut self, def: &ItemStruct, ns: &Namespace) {
fn ingest_struct(
&mut self,
def: &ItemStruct,
ns: &Namespace,
fieldlist: &IndexSet<QualifiedName>,
) {
// For this struct, work out whether it _could_ be safe as a POD.
let tyname = QualifiedName::new(ns, def.ident.clone());
let mut field_safety_problem = PodState::SafeToBePod;
let fieldlist = Self::get_field_types(def);
for ty_id in &fieldlist {
for ty_id in fieldlist.iter() {
match self.results.get(ty_id) {
None => {
field_safety_problem = PodState::UnsafeToBePod(format!(
Expand All @@ -171,7 +178,7 @@ impl ByValueChecker {
field_safety_problem = PodState::UnsafeToBePod(reason);
}
let mut my_details = StructDetails::new(field_safety_problem);
my_details.dependent_structs = fieldlist;
my_details.dependent_structs = fieldlist.clone();
self.results.insert(tyname, my_details);
}

Expand Down Expand Up @@ -200,7 +207,7 @@ impl ByValueChecker {
PodState::IsPod => {}
PodState::SafeToBePod => {
deets.state = PodState::IsPod;
requests.extend_from_slice(&deets.dependent_structs);
requests.extend(deets.dependent_structs.iter().cloned());
}
PodState::IsAlias(target_type) => {
alias_to_consider = Some(target_type.clone());
Expand Down Expand Up @@ -236,18 +243,6 @@ impl ByValueChecker {
)
}

fn get_field_types(def: &ItemStruct) -> Vec<QualifiedName> {
let mut results = Vec::new();
for f in &def.fields {
let fty = &f.ty;
if let Type::Path(p) = fty {
results.push(QualifiedName::from_type_path(p));
}
// TODO handle anything else which bindgen might spit out, e.g. arrays?
}
results
}

fn has_vtable(def: &ItemStruct) -> bool {
for f in &def.fields {
if f.ident.as_ref().map(|id| id == "vtable_").unwrap_or(false) {
Expand Down Expand Up @@ -285,7 +280,11 @@ mod tests {
}
};
let t_id = ty_from_ident(&t.ident);
bvc.ingest_struct(&t, &Namespace::new());
let fieldlist = [
QualifiedName::new_from_cpp_name("int32_t"),
QualifiedName::new_from_cpp_name("int64_t"),
];
bvc.ingest_struct(&t, &Namespace::new(), &fieldlist.into_iter().collect());
bvc.satisfy_requests(vec![t_id.clone()]).unwrap();
assert!(bvc.is_pod(&t_id));
}
Expand All @@ -299,15 +298,23 @@ mod tests {
b: i64,
}
};
bvc.ingest_struct(&t, &Namespace::new());
let fieldlist = [
QualifiedName::new_from_cpp_name("int32_t"),
QualifiedName::new_from_cpp_name("int64_t"),
];
bvc.ingest_struct(&t, &Namespace::new(), &fieldlist.into_iter().collect());
let t: ItemStruct = parse_quote! {
struct Bar {
a: Foo,
b: i64,
}
};
let fieldlist = [
QualifiedName::new_from_cpp_name("Foo"),
QualifiedName::new_from_cpp_name("int64_t"),
];
let t_id = ty_from_ident(&t.ident);
bvc.ingest_struct(&t, &Namespace::new());
bvc.ingest_struct(&t, &Namespace::new(), &fieldlist.into_iter().collect());
bvc.satisfy_requests(vec![t_id.clone()]).unwrap();
assert!(bvc.is_pod(&t_id));
}
Expand All @@ -321,8 +328,12 @@ mod tests {
b: i64,
}
};
let fieldlist = [
QualifiedName::new_from_cpp_name("std::unique_ptr"),
QualifiedName::new_from_cpp_name("int64_t"),
];
let t_id = ty_from_ident(&t.ident);
bvc.ingest_struct(&t, &Namespace::new());
bvc.ingest_struct(&t, &Namespace::new(), &fieldlist.into_iter().collect());
bvc.satisfy_requests(vec![t_id.clone()]).unwrap();
assert!(bvc.is_pod(&t_id));
}
Expand All @@ -336,8 +347,12 @@ mod tests {
b: i64,
}
};
let fieldlist = [
QualifiedName::new_from_cpp_name("std::string"),
QualifiedName::new_from_cpp_name("int64_t"),
];
let t_id = ty_from_ident(&t.ident);
bvc.ingest_struct(&t, &Namespace::new());
bvc.ingest_struct(&t, &Namespace::new(), &fieldlist.into_iter().collect());
assert!(bvc.satisfy_requests(vec![t_id]).is_err());
}
}
Loading