lib: Return error early if there are any error diagnostics. #330
Conversation
…n't have to do this themselves.
|
Added other kinda random fixups that I found as user of the library. |
| 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.
Probably you can move the has_target_arg to wrap the Clang::find block, so that it is somehow configurable?
| use clang::Diagnostic; | ||
| use clangll::*; | ||
|
|
||
| let mut any_error = false; |
There was a problem hiding this comment.
I think it would be easier to get context.translation_unit().diags() first, and if it is not empty, iterate it and return Err.
There was a problem hiding this comment.
In diags there can be warnings too, so we'd need to filter them.
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That doesn't quite fit with the rest of the builder methods, I think overriding it, albeit uncommon, should be fine?
| @@ -0,0 +1,6 @@ | |||
| // bindgen-flags: -- -x c++ --std=c++11 | |||
There was a problem hiding this comment.
I don't see how this test is related to this commit.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That being said that's probably a clang bug.
|
@bors-servo r=xidorn |
|
📌 Commit 8a60512 has been approved by |
|
⚡ Test exempted - status |
| 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.
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.
There was a problem hiding this comment.
I suggest we remove this hacky fixup and do a breaking version bump, then teach users to add what they need themselves.
|
On Thu, Feb 02, 2017 at 03:09:09PM -0800, Xidorn Quan wrote:
upsuper commented on this pull request.
> @@ -509,6 +509,25 @@ impl<'ctx> Bindings<'ctx> {
-> Result<Bindings<'ctx>, ()> {
let span = span.unwrap_or(DUMMY_SP);
+ // TODO: Make this path fixup 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() {
I suggest we remove this hacky fixup and do a breaking version bump, then teach users to add what they need themselves.
I'm not too fond on this because this is what allows C libraries to find
a bunch of standard headers.
I'd prefer adding an option to skip the path fixup if wanted. Would that
be fine?
|
|
I suppose libclang should have the same path searching behavior as the command line clang, so doing this doesn't add much value. Is that not true? Do you mean that libclang may not add system search path by default? Could you share some examples that things would not work without this fixup but works fine when using the command line clang? |
Nope, unfortunately. see #242 for an issue fixed by this path fixup. I'm sure there are more in the backlog of issues and the old repo. |
|
The issue doesn't mention the code and the arguments they use (thus not reproducible), so it isn't very convincing to me. It is possible that some environment or argument they provide (or lack) affects the search path decision, and the hacky fixup happens to fill the gap. |
r? @fitzgen
This is a rebase of #280