Skip to content

lib: Return error early if there are any error diagnostics. #330

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
Dec 12, 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
25 changes: 0 additions & 25 deletions bindgen/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,31 +42,6 @@ pub fn main() {
_ => {}
}

if let Some(clang) = clang_sys::support::Clang::find(None) {
let has_clang_args =
bind_args.iter().rposition(|arg| *arg == "--").is_some();
if !has_clang_args {
bind_args.push("--".to_owned());
}

// If --target is specified, assume caller knows what they're doing and
// don't mess with
// include paths for them
let has_target_arg = bind_args.iter()
.rposition(|arg| arg.starts_with("--target"))
.is_some();
if !has_target_arg {
// TODO: distinguish C and C++ paths? C++'s should be enough, I
// guess.
for path in clang.cpp_search_paths.into_iter() {
if let Ok(path) = path.into_os_string().into_string() {
bind_args.push("-isystem".to_owned());
bind_args.push(path);
}
}
}
}

match builder_from_flags(bind_args.into_iter()) {
Ok((builder, output)) => {
let mut bindings = builder.generate()
Expand Down
2 changes: 1 addition & 1 deletion libbindgen/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ license = "BSD-3-Clause"
name = "libbindgen"
readme = "README.md"
repository = "https://github.com/servo/rust-bindgen"
version = "0.1.1"
version = "0.1.2"
workspace = ".."

[dev-dependencies]
Expand Down
40 changes: 35 additions & 5 deletions libbindgen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,8 @@ impl Builder {
/// Set the input C/C++ header.
pub fn header<T: Into<String>>(mut self, header: T) -> Builder {
let header = header.into();
self.options.input_header = Some(header.clone());
self.clang_arg(header)
self.options.input_header = Some(header);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the only effect of this function is setting input_header, I guess it probably makes sense to assert that input_header is None before assigning it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't quite fit with the rest of the builder methods, I think overriding it, albeit uncommon, should be fine?

self
}

/// Generate a C/C++ file that includes the header and has dummy uses of
Expand Down Expand Up @@ -504,13 +504,36 @@ impl<'ctx> Bindings<'ctx> {
///
/// Deprecated - use a `Builder` instead
#[deprecated]
pub fn generate(options: BindgenOptions,
pub fn generate(mut options: BindgenOptions,
span: Option<Span>)
-> Result<Bindings<'ctx>, ()> {
let span = span.unwrap_or(DUMMY_SP);

// TODO: Make this path fixup configurable?
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably you can move the has_target_arg to wrap the Clang::find block, so that it is somehow configurable?

if let Some(clang) = clang_sys::support::Clang::find(None) {
// If --target is specified, assume caller knows what they're doing
// and don't mess with include paths for them
let has_target_arg = options.clang_args.iter()
.rposition(|arg| arg.starts_with("--target"))
.is_some();
if !has_target_arg {
// TODO: distinguish C and C++ paths? C++'s should be enough, I
// guess.
for path in clang.cpp_search_paths.into_iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I start wondering (again) why do we need this fixup. What would happen if we don't have it? It doesn't seem to me it would fail to build without this on macOS (since I can run stylo bindgen with --target=x86_64-apple-darwin -stdlib=libc++ specified), and we've already disabled it for stylo build on Windows (because we always specify --target). So Linux?

My guess is that the user of this library should always specify things they need, e.g. -stdlib=libc++ or -stdlib=libstdc++, rather than we doing hacky fixup under the hood.

Also, other than --target, there are many clang arguments can affect search path selection as well, e.g. -stdlib, and -mmacos-version-min on macOS, which means the paths here may not match what user expect, and thus add noise.

The recent example of bustage with MACOSX_DEPOLYMENT_TARGET environment is probably another example of harmfulness this kind of implicit fixup adds. If we have -stdlib=libc++ specified since the very beginning because of the lack of this fixup, that may never happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we remove this hacky fixup and do a breaking version bump, then teach users to add what they need themselves.

if let Ok(path) = path.into_os_string().into_string() {
options.clang_args.push("-isystem".to_owned());
options.clang_args.push(path);
}
}
}
}

if let Some(h) = options.input_header.as_ref() {
options.clang_args.push(h.clone())
}

let mut context = BindgenContext::new(options);
parse(&mut context);
try!(parse(&mut context));

let module = ast::Mod {
inner: span,
Expand Down Expand Up @@ -624,14 +647,20 @@ pub fn parse_one(ctx: &mut BindgenContext,
}

/// Parse the Clang AST into our `Item` internal representation.
fn parse(context: &mut BindgenContext) {
fn parse(context: &mut BindgenContext) -> Result<(), ()> {
use clang::Diagnostic;
use clangll::*;

let mut any_error = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be easier to get context.translation_unit().diags() first, and if it is not empty, iterate it and return Err.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In diags there can be warnings too, so we'd need to filter them.

for d in context.translation_unit().diags().iter() {
let msg = d.format(Diagnostic::default_opts());
let is_err = d.severity() >= CXDiagnostic_Error;
println!("{}, err: {}", msg, is_err);
any_error |= is_err;
}

if any_error {
return Err(());
}

let cursor = context.translation_unit().cursor();
Expand All @@ -646,6 +675,7 @@ fn parse(context: &mut BindgenContext) {

assert!(context.current_module() == context.root_module(),
"How did this happen?");
Ok(())
}

/// Extracted Clang version data
Expand Down
12 changes: 12 additions & 0 deletions libbindgen/tests/expectations/tests/dash_language.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/* automatically generated by rust-bindgen */


#![allow(non_snake_case)]


#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct Foo<T> {
pub bar: ::std::os::raw::c_int,
pub _phantom_0: ::std::marker::PhantomData<T>,
}
6 changes: 6 additions & 0 deletions libbindgen/tests/headers/dash_language.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// bindgen-flags: -- -x c++ --std=c++11
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this test is related to this commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is related because before that, that gives an error due to us sending the header before -x c++, and treats it as C because it's a .h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That being said that's probably a clang bug.


template<typename T>
struct Foo {
int bar;
};
2 changes: 2 additions & 0 deletions libbindgen/tests/headers/empty_template_param_name.hpp
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// bindgen-flags: -- -std=c++11

template<typename...> using __void_t = void;

template<typename _Iterator, typename = __void_t<>>
Expand Down