objc: Support method arguments #475

scoopr · 2017-02-04T00:30:14Z

Welp, I attempted to get the method arguments working.

I must confess, that I have not ran this against actual objective c code yet though.

highfive · 2017-02-04T00:30:17Z

Warning

These commits modify unsafe code. Please review it carefully!

bors-servo · 2017-02-04T04:14:03Z

☔ The latest upstream changes (presumably #473) made this pull request unmergeable. Please resolve the merge conflicts.

emilio

Looks great! I have a few comments, nothing excessively important. Thanks!

emilio · 2017-02-04T10:40:55Z

src/ir/objc.rs

+    }
+
+    /// Formats the method call
+    pub fn format_method_call(&self, args: &Vec<String>) -> String {


nit: This can take a &[String].

Also, I'm not super-fond on the name of this method, but I can't suggest anything clearly better so oh well... :)

The hard things in computer science :)

emilio · 2017-02-04T10:41:20Z

src/ir/objc.rs

+    /// Formats the method call
+    pub fn format_method_call(&self, args: &Vec<String>) -> String {
+        let split_name: Vec<&str> =
+            self.name.split(':').filter(|p| p.len() > 0).collect();


nit: !p.is_empty() instead of p.len > 0?

emilio · 2017-02-04T10:42:44Z

src/ir/objc.rs

+        if args.len() != split_name.len() {
+            panic!("Incorrect method name or arguments for objc method, {} != {}",
+                   args.len(),
+                   split_name.len());


Probably worth printing the argument lists?

emilio · 2017-02-04T10:43:09Z

src/ir/objc.rs

+            .map(|parts| format!("{}:{} ", parts.0, parts.1))
+            .collect::<Vec<_>>()
+            .join("")
+


nit: extra newline.

emilio · 2017-02-04T10:45:29Z

src/ir/objc.rs

 use clang;
 use clang_sys::CXChildVisit_Continue;
 use clang_sys::CXCursor_ObjCInstanceMethodDecl;
+use super::context::BindgenContext;
+use super::function::FunctionSig;
+// use clang_sys::CXCursor_ObjCSuperClassRef;


nit: Remove for now?

emilio · 2017-02-04T10:48:38Z

src/codegen/mod.rs

+                .map(|ref arg| {
+                    let pat = arg.pat.clone().unwrap();
+                    match pat.node {
+                        ast::PatKind::Ident(_, spanning, _) => {


not super awesome, but I guess it works. Alternatively, maybe get an Option<&mut Vec<String>> in fnsig_arguments that gets the argument names? That could also get used by arguments_for_signature in src/codegen/helpers.rs.

Yeah, that actually makes perfect sense, and I quickly tried to implement that, but I'm still a bit of a noob in rust, and couldn't make mutable thing within option, modified in a map()'s FnMut thing to work, I guess I have some learning to do here..

emilio · 2017-02-04T10:49:28Z

src/codegen/mod.rs

+            let arg_names: Vec<_> = fn_args.iter()
+                .map(|ref arg| {
+                    let pat = arg.pat.clone().unwrap();
+                    match pat.node {


I think you should be able to avoid this clone if you use match arg.pat.node and use ref spanning below.

emilio

One last thing I overlooked, can be done in a followup if it's too much work for some reason.

Thanks again!

emilio · 2017-02-05T20:22:04Z

src/ir/function.rs

@@ -85,6 +85,7 @@ fn get_abi(cc: CXCallingConv) -> abi::Abi {
        CXCallingConv_X86FastCall => abi::Abi::Fastcall,
        CXCallingConv_AAPCS => abi::Abi::Aapcs,
        CXCallingConv_X86_64Win64 => abi::Abi::Win64,
+        CXCallingConv_Invalid => abi::Abi::C, // TODO:


Instead of this, could we make this function return Option<abi::Abi>, commenting that it should only return None for objective C stuff, and asserting that in the caller?

Like this? (as separate commit)

emilio

Yup, pretty much! :)

emilio · 2017-02-05T22:19:44Z

src/ir/function.rs

@@ -228,6 +228,11 @@ impl FunctionSig {
        let ret = Item::from_ty_or_ref(ty_ret_type, None, None, ctx);
        let abi = get_abi(ty.call_conv());

+        if abi == None {


nit: if abi.is_none().

emilio · 2017-02-05T22:21:04Z

src/ir/function.rs

@@ -228,6 +228,11 @@ impl FunctionSig {
        let ret = Item::from_ty_or_ref(ty_ret_type, None, None, ctx);
        let abi = get_abi(ty.call_conv());

+        if abi == None {
+            assert!(cursor.kind() == CXCursor_ObjCInstanceMethodDecl,


nit: if you use assert_eq! instead, the panic message will also show the failing cursor kind.

emilio · 2017-02-06T18:24:44Z

@bors-servo r+

Thanks!

bors-servo · 2017-02-06T18:24:45Z

📌 Commit bc87b2a has been approved by emilio

bors-servo · 2017-02-06T18:24:48Z

⌛ Testing commit bc87b2a with merge 7fa654c...

objc: Support method arguments Welp, I attempted to get the method arguments working. I must confess, that I have not ran this against actual objective c code yet though.

bors-servo · 2017-02-06T20:28:23Z

☀️ Test successful - status-travis
Approved by: emilio
Pushing 7fa654c to master...

highfive added the S-awaiting-review label Feb 4, 2017

highfive added the S-needs-rebase label Feb 4, 2017

emilio approved these changes Feb 4, 2017

View reviewed changes

objc: Support method arguments

8128b2a

scoopr force-pushed the objc branch from 2193986 to 8128b2a Compare February 5, 2017 20:11

emilio reviewed Feb 5, 2017

View reviewed changes

emilio approved these changes Feb 5, 2017

View reviewed changes

scoopr force-pushed the objc branch from bfe0d1d to 74ec28e Compare February 6, 2017 07:58

Make abi optional for FunctionSig

bc87b2a

scoopr force-pushed the objc branch from 74ec28e to bc87b2a Compare February 6, 2017 08:01

highfive assigned emilio Feb 6, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Feb 6, 2017

bors-servo merged commit bc87b2a into rust-lang:master Feb 6, 2017

highfive removed the S-awaiting-merge label Feb 6, 2017

objc: Support method arguments #475

objc: Support method arguments #475

Uh oh!

Conversation

scoopr commented Feb 4, 2017

Uh oh!

highfive commented Feb 4, 2017

Uh oh!

bors-servo commented Feb 4, 2017

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio commented Feb 6, 2017

Uh oh!

bors-servo commented Feb 6, 2017

Uh oh!

bors-servo commented Feb 6, 2017

Uh oh!

bors-servo commented Feb 6, 2017

Uh oh!

Uh oh!