Skip to content

Commit

Permalink
Implement most of the C++ special member function rules
Browse files Browse the repository at this point in the history 
This now tracks most of the information about which C++ special member
functions are implicit/explicit/deleted/etc for most of the common
cases. This information was needed in several places, which were each
using different approximations that failed in different ways, so unify
those to get it all working. Also add a bunch of tests around the
various cases to keep this working.

This assumes that any non-analyzed types (except some built-in ones
which are handled specially) have no implicit special member functions,
instead of the previous behavior which assumed they all existed if the
analyzed type had explicit declarations. This should generate
functional code for more situations, but it will skip some optional
things (such as moveit traits and make_unique) for additional types. If
you run into issues with those things disappearing after this change,
make sure all dependencies of the type (superclasses and member types)
have a `generate!`/`generate_pod!`.

Added TODOs for the following unhandled parts:
* google#815 (this is a Clang warning anyways, TODOs show all
  the places to change to fix it)
* google#816 (this just means we ignore some implicit
  constructors which do exist)

Also added TODOs related to the followig issues, which limit what can be
tested but aren't made better or worse by this change:
* google#832 (this one affects lots of areas)
* google#829 (this one's pretty prone to unexpected behavior)

Also fixed some existing bugs which are surfaced by generating more
trait implementations for types in the existing tests:
* Use the correct C++ name for destructors of nested types
* Handle trait methods for types that end up ignored
  • Loading branch information
@bsilver8192
bsilver8192 committed Feb 28, 2022
1 parent 21a9a59 commit 8943de2
Show file tree
Hide file tree
Showing 15 changed files with 2,077 additions and 628 deletions.
19 changes: 14 additions & 5 deletions engine/src/conversion/analysis/abstract_types.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use super::{
};
use crate::conversion::api::Api;
use crate::conversion::{
analysis::fun::FnStructAnalysis,
api::TypeKind,
error_reporter::{convert_apis, convert_item_apis},
ConvertError,
Expand All @@ -44,7 +45,7 @@ pub(crate) fn mark_types_abstract(mut apis: Vec<Api<FnPrePhase>>) -> Vec<Api<FnP
for mut api in apis.iter_mut() {
match &mut api {
Api::Struct { analysis, name, .. } if abstract_types.contains(&name.name) => {
analysis.kind = TypeKind::Abstract;
analysis.pod.kind = TypeKind::Abstract;
}
_ => {}
}
Expand All @@ -60,7 +61,11 @@ pub(crate) fn mark_types_abstract(mut apis: Vec<Api<FnPrePhase>>) -> Vec<Api<FnP
for mut api in apis.iter_mut() {
match &mut api {
Api::Struct {
analysis: PodAnalysis { bases, kind, .. },
analysis:
FnStructAnalysis {
pod: PodAnalysis { bases, kind, .. },
..
},
..
} if *kind != TypeKind::Abstract && (!abstract_types.is_disjoint(bases)) => {
*kind = TypeKind::Abstract;
Expand All @@ -80,7 +85,7 @@ pub(crate) fn mark_types_abstract(mut apis: Vec<Api<FnPrePhase>>) -> Vec<Api<FnP
Api::Function {
analysis:
FnAnalysis {
kind: FnKind::Method(self_ty, MethodKind::MakeUnique | MethodKind::Constructor)
kind: FnKind::Method(self_ty, MethodKind::MakeUnique | MethodKind::Constructor | MethodKind::DefaultConstructor)
| FnKind::TraitMethod{ kind: TraitMethodKind::CopyConstructor | TraitMethodKind::MoveConstructor, impl_for: self_ty, ..},
..
},
Expand All @@ -101,8 +106,12 @@ pub(crate) fn mark_types_abstract(mut apis: Vec<Api<FnPrePhase>>) -> Vec<Api<FnP
convert_item_apis(apis, &mut results, |api| match api {
Api::Struct {
analysis:
PodAnalysis {
kind: TypeKind::Abstract,
FnStructAnalysis {
pod:
PodAnalysis {
kind: TypeKind::Abstract,
..
},
..
},
..
Expand Down
9 changes: 5 additions & 4 deletions engine/src/conversion/analysis/constructor_deps.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,8 @@ use crate::{
types::QualifiedName,
};

use super::{
fun::{FnAnalysis, FnKind, FnPhase, FnPrePhase, PodAndDepAnalysis, TraitMethodKind},
pod::PodAnalysis,
use super::fun::{
FnAnalysis, FnKind, FnPhase, FnPrePhase, FnStructAnalysis, PodAndDepAnalysis, TraitMethodKind,
};

/// We've now analyzed all functions (including both implicit and explicit
Expand Down Expand Up @@ -54,9 +53,10 @@ pub(crate) fn decorate_types_with_constructor_deps(
fn decorate_struct(
name: ApiName,
details: Box<StructDetails>,
pod: PodAnalysis,
fn_struct: FnStructAnalysis,
constructors_and_allocators_by_type: &mut HashMap<QualifiedName, Vec<QualifiedName>>,
) -> Result<Box<dyn Iterator<Item = Api<FnPhase>>>, ConvertErrorWithContext> {
let pod = fn_struct.pod;
let is_abstract = matches!(pod.kind, TypeKind::Abstract);
let constructor_and_allocator_deps = if is_abstract || pod.is_generic {
Vec::new()
Expand All @@ -71,6 +71,7 @@ fn decorate_struct(
analysis: PodAndDepAnalysis {
pod,
constructor_and_allocator_deps,
constructors: fn_struct.constructors,
},
})))
}
Expand Down
18 changes: 13 additions & 5 deletions engine/src/conversion/analysis/deps.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::{
};

use super::{
fun::{FnPhase, FnPrePhase, PodAndDepAnalysis},
fun::{FnPhase, FnPrePhase, FnStructAnalysis, PodAndDepAnalysis},
pod::PodAnalysis,
tdef::TypedefAnalysis,
};
Expand All @@ -44,13 +44,18 @@ impl HasDependencies for Api<FnPrePhase> {
} => Box::new(old_tyname.iter().chain(deps.iter())),
Api::Struct {
analysis:
PodAnalysis {
kind: TypeKind::Pod,
field_types,
FnStructAnalysis {
pod:
PodAnalysis {
kind: TypeKind::Pod,
bases,
field_types,
..
},
..
},
..
} => Box::new(field_types.iter()),
} => Box::new(field_types.iter().chain(bases.iter())),
Api::Function { analysis, .. } => Box::new(analysis.deps.iter()),
Api::Subclass {
name: _,
Expand Down Expand Up @@ -81,15 +86,18 @@ impl HasDependencies for Api<FnPhase> {
pod:
PodAnalysis {
kind: TypeKind::Pod,
bases,
field_types,
..
},
constructor_and_allocator_deps,
..
},
..
} => Box::new(
field_types
.iter()
.chain(bases.iter())
.chain(constructor_and_allocator_deps.iter()),
),
Api::Struct {
Expand Down
219 changes: 77 additions & 142 deletions engine/src/conversion/analysis/fun/implicit_constructor_rules.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,164 +14,99 @@

//! Module which understands C++ constructor synthesis rules.

/// Output of the C++ rules about what implicit constructors should be generated.
#[cfg_attr(test, derive(Eq, PartialEq))]
#[derive(Debug)]
pub(super) struct ImplicitConstructorsNeeded {
pub(super) default_constructor: bool,
pub(super) copy_constructor_taking_t: bool,
pub(super) copy_constructor_taking_const_t: bool,
pub(super) move_constructor: bool,
use crate::conversion::api::CppVisibility;

/// Indicates what we found out about a category of special member function.
///
/// In the end, we only care whether it's public and exists, but we track a bit more information to
/// support determining the information for dependent classes.
#[derive(Debug, Copy, Clone)]
pub(super) enum SpecialMemberFound {
/// This covers being deleted in any way:
/// * Explicitly deleted
/// * Implicitly defaulted when that means being deleted
/// * Explicitly defaulted when that means being deleted
///
/// It also covers not being either user declared or implicitly defaulted.
NotPresent,
/// Implicit special member functions, indicated by this, are always public.
Implicit,
/// This covers being explicitly defaulted (when that is not deleted) or being user-defined.
Explicit(CppVisibility),
}

/// Input to the C++ rules about which implicit constructors are generated.
#[derive(Default, Debug)]
pub(super) struct ExplicitItemsFound {
pub(super) move_constructor: bool,
pub(super) copy_constructor: bool,
pub(super) any_other_constructor: bool,
pub(super) any_bases_or_fields_lack_const_copy_constructors: bool,
pub(super) any_bases_or_fields_have_deleted_or_inaccessible_copy_constructors: bool,
pub(super) destructor: bool,
pub(super) any_bases_have_deleted_or_inaccessible_destructors: bool,
pub(super) copy_assignment_operator: bool,
pub(super) move_assignment_operator: bool,
pub(super) has_rvalue_reference_fields: bool,
pub(super) any_field_or_base_not_understood: bool,
}

pub(super) fn determine_implicit_constructors(
explicits: ExplicitItemsFound,
) -> ImplicitConstructorsNeeded {
if explicits.any_field_or_base_not_understood {
// Don't generate anything
return ImplicitConstructorsNeeded {
default_constructor: false,
copy_constructor_taking_t: false,
copy_constructor_taking_const_t: false,
move_constructor: false,
};
impl SpecialMemberFound {
/// Returns whether code outside of subclasses can call this special member function.
pub fn callable_any(&self) -> bool {
matches!(self, Self::Explicit(CppVisibility::Public) | Self::Implicit)
}

let any_constructor =
explicits.copy_constructor || explicits.move_constructor || explicits.any_other_constructor;
// If no user-declared constructors of any kind are provided for a class type (struct, class, or union),
// the compiler will always declare a default constructor as an inline public member of its class.
let default_constructor = !any_constructor;

// If no user-defined copy constructors are provided for a class type (struct, class, or union),
// the compiler will always declare a copy constructor as a non-explicit inline public member of its class.
// This implicitly-declared copy constructor has the form T::T(const T&) if all of the following are true:
// each direct and virtual base B of T has a copy constructor whose parameters are const B& or const volatile B&;
// each non-static data member M of T of class type or array of class type has a copy constructor whose parameters are const M& or const volatile M&.

// The implicitly-declared or defaulted copy constructor for class T is defined as deleted if any of the following conditions are true:
// T is a union-like class and has a variant member with non-trivial copy constructor; // we don't support unions anyway
// T has a user-defined move constructor or move assignment operator (this condition only causes the implicitly-declared, not the defaulted, copy constructor to be deleted).
// T has non-static data members that cannot be copied (have deleted, inaccessible, or ambiguous copy constructors);
// T has direct or virtual base class that cannot be copied (has deleted, inaccessible, or ambiguous copy constructors);
// T has direct or virtual base class with a deleted or inaccessible destructor;
// T has a data member of rvalue reference type;
let copy_constructor_is_deleted = explicits.move_constructor
|| explicits.move_assignment_operator
|| explicits.any_bases_or_fields_have_deleted_or_inaccessible_copy_constructors
|| explicits.any_bases_have_deleted_or_inaccessible_destructors
|| explicits.has_rvalue_reference_fields;
/// Returns whether code in a subclass can call this special member function.
pub fn callable_subclass(&self) -> bool {
matches!(
self,
Self::Explicit(CppVisibility::Public)
| Self::Explicit(CppVisibility::Protected)
| Self::Implicit
)
}

let (copy_constructor_taking_const_t, copy_constructor_taking_t) =
if explicits.copy_constructor || copy_constructor_is_deleted {
(false, false)
} else if explicits.any_bases_or_fields_lack_const_copy_constructors {
(false, true)
} else {
(true, false)
};
/// Returns whether this exists at all. Note that this will return true even if it's private,
/// which is generally not very useful, but does come into play for some rules around which
/// default special member functions are deleted vs don't exist.
pub fn exists(&self) -> bool {
matches!(self, Self::Explicit(_) | Self::Implicit)
}

// If no user-defined move constructors are provided for a class type (struct, class, or union), and all of the following is true:
// there are no user-declared copy constructors;
// there are no user-declared copy assignment operators;
// there are no user-declared move assignment operators;
// there is no user-declared destructor.
// then the compiler will declare a move constructor
let move_constructor = !(explicits.move_constructor
|| explicits.copy_constructor
|| explicits.destructor
|| explicits.copy_assignment_operator
|| explicits.move_assignment_operator);
pub fn exists_implicit(&self) -> bool {
matches!(self, Self::Implicit)
}

ImplicitConstructorsNeeded {
default_constructor,
copy_constructor_taking_t,
copy_constructor_taking_const_t,
move_constructor,
pub fn exists_explicit(&self) -> bool {
matches!(self, Self::Explicit(_))
}
}

#[cfg(test)]
mod tests {
use super::determine_implicit_constructors;

use super::ExplicitItemsFound;

#[test]
fn test_simple() {
let inputs = ExplicitItemsFound::default();
let outputs = determine_implicit_constructors(inputs);
assert!(outputs.default_constructor);
assert!(outputs.copy_constructor_taking_const_t);
assert!(!outputs.copy_constructor_taking_t);
assert!(outputs.move_constructor);
}
/// Information about which special member functions exist based on the C++ rules.
///
/// Not all of this information is used directly, but we need to track it to determine the
/// information we do need for classes which are used as members or base classes.
#[derive(Debug, Copy, Clone)]
pub(super) struct ItemsFound {
pub(super) default_constructor: SpecialMemberFound,
pub(super) destructor: SpecialMemberFound,
pub(super) const_copy_constructor: SpecialMemberFound,
/// Remember that [`const_copy_constructor`] may be used in place of this if it exists.
pub(super) non_const_copy_constructor: SpecialMemberFound,
pub(super) move_constructor: SpecialMemberFound,
}

#[test]
fn test_with_destructor() {
let inputs = ExplicitItemsFound {
destructor: true,
..Default::default()
};
let outputs = determine_implicit_constructors(inputs);
assert!(outputs.default_constructor);
assert!(outputs.copy_constructor_taking_const_t);
assert!(!outputs.copy_constructor_taking_t);
assert!(!outputs.move_constructor);
impl ItemsFound {
/// Returns whether we should generate a default constructor wrapper, because bindgen won't do
/// one for the implicit default constructor which exists.
pub(super) fn implicit_default_constructor_needed(&self) -> bool {
self.default_constructor.exists_implicit()
}

#[test]
fn test_with_pesky_base() {
let inputs = ExplicitItemsFound {
any_bases_or_fields_lack_const_copy_constructors: true,
..Default::default()
};
let outputs = determine_implicit_constructors(inputs);
assert!(outputs.default_constructor);
assert!(!outputs.copy_constructor_taking_const_t);
assert!(outputs.copy_constructor_taking_t);
assert!(outputs.move_constructor);
/// Returns whether we should generate a copy constructor wrapper, because bindgen won't do one
/// for the implicit copy constructor which exists.
pub(super) fn implicit_copy_constructor_needed(&self) -> bool {
let any_implicit_copy = self.const_copy_constructor.exists_implicit()
|| self.non_const_copy_constructor.exists_implicit();
let no_explicit_copy = !(self.const_copy_constructor.exists_explicit()
|| self.non_const_copy_constructor.exists_explicit());
any_implicit_copy && no_explicit_copy
}

#[test]
fn test_with_user_defined_move_constructor() {
let inputs = ExplicitItemsFound {
move_constructor: true,
..Default::default()
};
let outputs = determine_implicit_constructors(inputs);
assert!(!outputs.default_constructor);
assert!(!outputs.copy_constructor_taking_const_t);
assert!(!outputs.copy_constructor_taking_t);
assert!(!outputs.move_constructor);
/// Returns whether we should generate a move constructor wrapper, because bindgen won't do one
/// for the implicit move constructor which exists.
pub(super) fn implicit_move_constructor_needed(&self) -> bool {
self.move_constructor.exists_implicit()
}

#[test]
fn test_with_user_defined_misc_constructor() {
let inputs = ExplicitItemsFound {
any_other_constructor: true,
..Default::default()
};
let outputs = determine_implicit_constructors(inputs);
assert!(!outputs.default_constructor);
assert!(outputs.copy_constructor_taking_const_t);
assert!(!outputs.copy_constructor_taking_t);
assert!(outputs.move_constructor);
/// Returns whether we should generate a destructor wrapper, because bindgen won't do one for
/// the implicit destructor which exists.
pub(super) fn implicit_destructor_needed(&self) -> bool {
self.destructor.exists_implicit()
}
}
Loading

0 comments on commit 8943de2

Please sign in to comment.