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

Allow only generating layout tests #1761

Closed
wants to merge 1 commit into from
Closed

Allow only generating layout tests #1761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
53 changes: 43 additions & 10 deletions src/codegen/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ mod bitfield_unit_tests;
use self::helpers::attributes;
use self::struct_layout::StructLayoutTracker;

use super::BindgenOptions;
use super::{BindgenOptions, LayoutTests};

use ir::analysis::{HasVtable, Sizedness};
use ir::annotations::FieldAccessorKind;
Expand Down Expand Up @@ -98,6 +98,12 @@ fn root_import(
struct CodegenResult<'a> {
items: Vec<proc_macro2::TokenStream>,

/// Just the layout tests.
///
/// Used to implement `LayoutTests::EmitOnly`, and empty if that setting is
/// disabled (note that conversely `items` is always populted).
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: fix typo here.

It's a little unfortunate that items is written even in cases where we only want the test, but I think it would be a way bigger patch otherwise, since there are so many places to change. This didn't feel like the right tradeoff for this feature.

Maybe I'm wrong though. But... I'd argue it seems like the kind of thing to file as a potential optimization someone can do if they care or something. (E.g. please don't ask me to do this here unless it's very necessary 😅)

only_tests: Vec<proc_macro2::TokenStream>,

/// A monotonic counter used to add stable unique id's to stuff that doesn't
/// need to be referenced by anything.
codegen_id: &'a Cell<usize>,
Expand Down Expand Up @@ -147,6 +153,7 @@ impl<'a> CodegenResult<'a> {
fn new(codegen_id: &'a Cell<usize>) -> Self {
CodegenResult {
items: vec![],
only_tests: vec![],
saw_bindgen_union: false,
saw_incomplete_array: false,
saw_objc: false,
Expand Down Expand Up @@ -214,7 +221,10 @@ impl<'a> CodegenResult<'a> {
self.vars_seen.insert(name.into());
}

fn inner<F>(&mut self, cb: F) -> Vec<proc_macro2::TokenStream>
fn inner<F>(
Copy link
Member Author

Choose a reason for hiding this comment

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

This function is only changed to support the c++ namespace thing I did... which I don't think works.

&mut self,
cb: F,
) -> (Vec<proc_macro2::TokenStream>, Vec<proc_macro2::TokenStream>)
where
F: FnOnce(&mut Self),
{
Expand All @@ -228,7 +238,7 @@ impl<'a> CodegenResult<'a> {
self.saw_bitfield_unit |= new.saw_bitfield_unit;
self.saw_bindgen_union |= new.saw_bindgen_union;

new.items
(new.items, new.only_tests)
}
}

Expand Down Expand Up @@ -435,7 +445,7 @@ impl CodeGenerator for Module {
}

let mut found_any = false;
let inner_items = result.inner(|result| {
let (inner_items, inner_tests) = result.inner(|result| {
result.push(root_import(ctx, item));

let path = item.namespace_aware_canonical_path(ctx).join("::");
Expand All @@ -457,7 +467,7 @@ impl CodeGenerator for Module {
}

let name = item.canonical_name(ctx);
let ident = ctx.rust_ident(name);
let ident = ctx.rust_ident(&name);
result.push(if item.id() == ctx.root_module() {
quote! {
#[allow(non_snake_case, non_camel_case_types, non_upper_case_globals)]
Expand All @@ -472,6 +482,19 @@ impl CodeGenerator for Module {
}
}
});
if ctx.options().layout_tests == LayoutTests::EmitOnly &&
!result.only_tests.is_empty()
{
// XXX the following appears to work? Or should this just be unsupported.
Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty sure this doesn't actually work and it just managed to not cause errors.

let test_mod =
ctx.rust_ident(format!("__bindgen_test_mod_{}", name));
result.only_tests.push(quote! {
mod #test_mod {
use super::#ident::*;
#( #inner_tests )*
}
});
}
}
}

Expand Down Expand Up @@ -959,7 +982,7 @@ impl CodeGenerator for TemplateInstantiation {
// instantiation is opaque, then its presumably because we don't
// properly understand it (maybe because of specializations), and so we
// shouldn't emit layout tests either.
if !ctx.options().layout_tests || self.is_opaque(ctx, item) {
if !ctx.options().layout_tests.needed() || self.is_opaque(ctx, item) {
return;
}

Expand Down Expand Up @@ -1006,7 +1029,9 @@ impl CodeGenerator for TemplateInstantiation {
stringify!(#ident)));
}
};

if ctx.options().layout_tests == LayoutTests::EmitOnly {
result.only_tests.push(item.clone());
}
result.push(item);
}
}
Expand Down Expand Up @@ -1899,7 +1924,9 @@ impl CodeGenerator for CompInfo {
}
}

if ctx.options().layout_tests && !self.is_forward_declaration() {
if ctx.options().layout_tests.needed() &&
!self.is_forward_declaration()
{
if let Some(layout) = layout {
let fn_name =
format!("bindgen_test_layout_{}", canonical_ident);
Expand Down Expand Up @@ -1982,6 +2009,9 @@ impl CodeGenerator for CompInfo {
#( #check_field_offset )*
}
};
if ctx.options().layout_tests == LayoutTests::EmitOnly {
result.only_tests.push(item.clone());
}
result.push(item);
}
}
Expand Down Expand Up @@ -3848,8 +3878,11 @@ pub(crate) fn codegen(
&mut result,
&(),
);

result.items
if LayoutTests::EmitOnly == context.options().layout_tests {
result.only_tests
} else {
result.items
}
})
}

Expand Down
56 changes: 50 additions & 6 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,8 +374,14 @@ impl Builder {
})
.count();

if !self.options.layout_tests {
output_vector.push("--no-layout-tests".into());
match self.options.layout_tests {
LayoutTests::Emit => {}
LayoutTests::EmitNone => {
output_vector.push("--no-layout-tests".into())
}
LayoutTests::EmitOnly => {
output_vector.push("--only-layout-tests".into())
}
}

if self.options.impl_debug {
Expand Down Expand Up @@ -1088,8 +1094,8 @@ impl Builder {
}

/// Set whether layout tests should be generated.
pub fn layout_tests(mut self, doit: bool) -> Self {
self.options.layout_tests = doit;
pub fn layout_tests(mut self, doit: impl Into<LayoutTests>) -> Self {
self.options.layout_tests = doit.into();
self
}

Expand Down Expand Up @@ -1551,6 +1557,44 @@ impl Builder {
}
}

/// Setting for layout test generation.
#[derive(Debug, Clone, PartialEq)]
pub enum LayoutTests {
Copy link
Member Author

Choose a reason for hiding this comment

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

Naming bikeshed welcome here and where-ever.

/// Include layout tests in the generated bindings. The default.
Emit,
/// Don't include any layout tests.
EmitNone,
/// Only emit the layout tests.
///
/// The intended use case for this is for separating tests from the
/// generated bindings (as the tests are by nature non-portable, even if the
/// bindings otherwise would be).
///
/// When used in this manner, you're encouraged to provide an an otherwise
/// identical set of options (even though many of them are effectively
/// ignored when this is set).
EmitOnly,
}

impl LayoutTests {
pub(crate) fn needed(&self) -> bool {
match self {
LayoutTests::Emit | LayoutTests::EmitOnly => true,
LayoutTests::EmitNone => false,
}
}
}

impl From<bool> for LayoutTests {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is mostly so that existing calls to layout_tests don't break. I also think it's sensible though.

fn from(value: bool) -> Self {
if value {
LayoutTests::Emit
} else {
LayoutTests::EmitNone
}
}
}

/// Configuration options for generated bindings.
#[derive(Debug)]
struct BindgenOptions {
Expand Down Expand Up @@ -1649,7 +1693,7 @@ struct BindgenOptions {
disable_nested_struct_naming: bool,

/// True if we should generate layout tests for generated structures.
layout_tests: bool,
layout_tests: LayoutTests,

/// True if we should implement the Debug trait for C/C++ structures and types
/// that do not support automatically deriving Debug.
Expand Down Expand Up @@ -1894,7 +1938,7 @@ impl Default for BindgenOptions {
emit_ast: false,
emit_ir: false,
emit_ir_graphviz: None,
layout_tests: true,
layout_tests: LayoutTests::Emit,
impl_debug: false,
impl_partialeq: false,
derive_copy: true,
Expand Down
5 changes: 5 additions & 0 deletions src/options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,11 @@ where
.takes_value(true)
.multiple(true)
.number_of_values(1),
Arg::with_name("only-layout-tests")
.long("only-layout-tests")
.help(
"Only emit layout tests. Allows separating the tests from the bindings."
),
Arg::with_name("no-layout-tests")
.long("no-layout-tests")
.help("Avoid generating layout tests for any type."),
Expand Down