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

Jump to bottom

Added constructor return type for wasm32 target #1877

Merged
merged 3 commits into from Aug 25, 2020
Merged

Added constructor return type for wasm32 target #1877

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ed0127
Added constructor return type for wasm32 target
tuxmark5 Aug 21, 2020
b1cefe2
Optimized condition order, added regression test
tuxmark5 Aug 24, 2020
d446a4f
Added expectation test files for clang 3.9 and 4
tuxmark5 Aug 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/ir/context.rs
View file
Open in desktop
Expand Up @@ -577,6 +577,14 @@ If you encounter an error missing from this list, please file an issue or a PR!"
}
}

/// Returns `true` if the target architecture is wasm32
pub fn is_target_wasm32(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work if building with clang 4 or 3.9 (target_info will be null), but maybe that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt that wasm backend even existed for those versions of clang and as such I don't think what would be an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

We still need to update the test expectations for these two versions, as we run CI on those. To do that you need to add the file in tests/expectations/tests/libclang-3.9 (and same for 4).

With that bit, this patch looks great, thank you!

match self.target_info {
Some(ref ti) => ti.triple.starts_with("wasm32-"),
None => false,
}
}

/// Creates a timer for the current bindgen phase. If time_phases is `true`,
/// the timer will print to stderr when it is dropped, otherwise it will do
/// nothing.
Expand Down
10 changes: 9 additions & 1 deletion src/ir/function.rs
View file
Open in desktop
Expand Up @@ -495,7 +495,15 @@ impl FunctionSig {
} else {
ty.ret_type().ok_or(ParseError::Continue)?
};
let ret = Item::from_ty_or_ref(ty_ret_type, cursor, None, ctx);

let ret = if ctx.is_target_wasm32() && is_constructor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we switch the order here, is_constructor is cheaper to check, and should be false most of the time.

Also, can we add a test for this? Should be doable passing --target in the bindgen-args like tests/headers/win32-thiscall_1_0.hpp and others do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a commit which addresses both of these comments.

// Constructors in Clang wasm32 target return a pointer to the object
// being constructed.
let void = Item::builtin_type(TypeKind::Void, false, ctx);
Item::builtin_type(TypeKind::Pointer(void), false, ctx)
} else {
Item::from_ty_or_ref(ty_ret_type, cursor, None, ctx)
};

// Clang plays with us at "find the calling convention", see #549 and
// co. This seems to be a better fix than that commit.
Expand Down