-
Notifications
You must be signed in to change notification settings - Fork 747
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
self | ||
} | ||
|
||
/// Generate a C/C++ file that includes the header and has dummy uses of | ||
|
@@ -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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably you can move the |
||
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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 My guess is that the user of this library should always specify things they need, e.g. Also, other than The recent example of bustage with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be easier to get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In |
||
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(); | ||
|
@@ -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 | ||
|
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>, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// bindgen-flags: -- -x c++ --std=c++11 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how this test is related to this commit. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; |
There was a problem hiding this comment.
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 thatinput_header
isNone
before assigning it.There was a problem hiding this comment.
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?