Parse macro expressions. #219

+        // FIXME(emilio): LLVM 3.9 at least always include an extra token for no
+        // good reason (except if we're at EOF). So we do this kind of hack,
+        // where we skip known-to-cause problems trailing punctuation and
+        // trailing keywords.


Not just 3.9, I've observed this in 3.5 through 3.8. The problem is that the cursor extent is always off by one (even on EOF). You don't get an extra token on EOF, because, well, you're at EOF.

That's really unfortunate :(

After inspecting a bit clang's source, the source range for macro expansions comes from deep in the lexer. That's more that what I want to chew in right now inside LLVM internals, so I'll open a bug for it.

Wasn't that hard actually.

LLVM bug (open since 2011): https://llvm.org/bugs/show_bug.cgi?id=9069
Patch: https://reviews.llvm.org/D26446

Fixed in trunk :)

llvm-mirror/clang@3b61b92

jethrogb · 2016-11-06T17:31:31Z

src/clang.rs

+                    CXToken_Literal => token::Kind::Literal,
+                    CXToken_Identifier => token::Kind::Identifier,
+                    CXToken_Keyword => token::Kind::Keyword,
+                    CXToken_Comment => token::Kind::Comment,


You should strip out comments (here or somewhere else before calling Identifier::macro_definition).

Huh, I saw the other bindgen doing that, but is there a good reason to? I expected cexpr to take care of them... Or do comments before the definition get included in the extent?

Thanks anyway, I'll add a test for it :)

From the docs:

Returns an error if the replacement is not a valid expression, if called on most function-like macros, or if the token stream contains comments, keywords or unknown identifiers.

If you think cexpr should take care of it I can see if I can change that.

jethrogb · 2016-11-06T17:46:12Z

Wow, that was fast :)

There is one unfortunate thing about C macros evaluating into integers: we don't know what type they should be for convienent usage with the rest of the definitions. If some macros are always intended to be passed into a function that takes a short, the type of the constant should ideally be short. If it's not, you're risking integer type casting all over the Rust code (or you need to manually edit the bindgen output).

Upstream selects types based on the highest bit set in the constant, and it allows to user to specify the specific type in each case. This is kind of a kludge but the best I could come up with.

emilio · 2016-11-06T17:50:28Z

Yep, I prefer to keep it as simple as possible and don't try to be uber-smart about macro types, so I prefer to keep UInt/Int in general, and longs if out of range, to keep with the usual C semantics.

Seems easy enough to re-export them casting as other type or similar if needed, and it simplifies quite a bit of code.

jethrogb · 2016-11-06T18:42:17Z

Being able to do something about this in an automated fashion is blocking my adoption of this fork in mbedtls-auto-sys (along with several other features I think).

I'd be ok with post-processing the AST in my build script, but

I can't distinguish in the Rust AST between C macros and C constants
bindgen won't let me modify the AST and then write it out

emilio · 2016-11-06T18:50:41Z

You mean macro_int_types?

jethrogb · 2016-11-06T18:51:35Z

Yes, sorry, I meant to highlight that line.

emilio · 2016-11-06T19:33:46Z

What about the API I added in the last commit, does something like that fullfill your needs? I think it's nicer and more generic.

jethrogb · 2016-11-06T19:36:25Z

Yeah, this looks great, thanks!

emilio · 2016-11-06T19:40:26Z

Heh, in this case it felt a bit more ugly because I needed to do chooser.as_ref() anyway, I don't think it matters much.

bors-servo · 2016-11-08T01:52:51Z

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

fitzgen

LGTM, just a few questions and nitpicks :)

Thanks!

fitzgen · 2016-11-08T19:07:48Z

src/ir/context.rs

-    parsed_macros: HashSet<String>,
+    /// hard errors while parsing duplicated macros, as well to allow macro
+    /// expression parsing.
+    parsed_macros: HashMap<Vec<u8>, cexpr::expr::EvalResult>,


Why did this become Vec<u8> instead of String?

It's what cexpr wants.

fitzgen · 2016-11-08T19:11:05Z

src/ir/int.rs

+    /// range.
+    ///
+    /// The boolean means a whether this is a signed integer type or not.
+    Custom(&'static str, bool),


Custom { name: &'static str, is_signed: bool, }

If you want.

Sure, sounds good.

fitzgen · 2016-11-08T19:14:38Z

src/ir/var.rs

+                // identifier of a token comes straight from clang, and we
+                // enforce utf8 there, so we should have already panicked at
+                // this point.
+                let name = String::from_utf8(id).unwrap();


Comment about why unwrapping is safe should become the str literal for .expect("blah blah") instead of using .unwrap() here.

Hmm... I don't use to use big strings in expect(), I'll do if it looks pretty :)

fitzgen · 2016-11-08T19:18:47Z

src/ir/var.rs

+
+                        let kind = match kind {
+                            Some(kind) => kind,
+                            None => {


If you want:

let kind = ctx.options().type_chooser .and_then(|ch| ch.int_macro(&name, value)) .unwrap_or_else(|| { // <the stuff in this `None => {...}` match arm> });

Ok, that looks finer than what I thought. Will do

fitzgen · 2016-11-08T19:22:33Z

tests/expectations/tests/jsval_layout_opaque.rs

@@ -24,8 +24,9 @@ impl <T> ::std::clone::Clone for __BindgenUnionField<T> {
    fn clone(&self) -> Self { Self::new() }
 }
 impl <T> ::std::marker::Copy for __BindgenUnionField<T> { }
-pub const JSVAL_ALIGNMENT: ::std::os::raw::c_uint = 8;


What happened to this?

It's a alignas(8), so we were parsing it by chance.

emilio · 2016-11-08T20:01:16Z

Thanks for the review Nick! :)

@bors-servo r=fitzgen

bors-servo · 2016-11-08T20:01:17Z

📌 Commit 4ee31ce has been approved by fitzgen

bors-servo · 2016-11-09T01:35:27Z

⌛ Testing commit 4ee31ce with merge 8d83bdf...

Parse macro expressions. Clang is trolling me really hard so I'm going to see if the extra token I'm always getting is LLVM 3.9 specific.

bors-servo · 2016-11-09T02:06:01Z

☀️ Test successful - status-travis

highfive added the S-awaiting-review label Nov 6, 2016

bors-servo pushed a commit that referenced this pull request Nov 6, 2016

Auto merge of #219 - emilio:macros, r=<try>

9811b01

Parse macro expressions. Clang is trolling me really hard so I'm going to see if the extra token I'm always getting is LLVM 3.9 specific.

highfive added the S-tests-failed label Nov 6, 2016

emilio force-pushed the macros branch from e0a0b1d to f707d0f Compare November 6, 2016 14:11

highfive removed the S-tests-failed label Nov 6, 2016

emilio force-pushed the macros branch from f707d0f to 3c2af5e Compare November 6, 2016 14:14

bors-servo pushed a commit that referenced this pull request Nov 6, 2016

Auto merge of #219 - emilio:macros, r=<try>

ee85cd8

Parse macro expressions. Clang is trolling me really hard so I'm going to see if the extra token I'm always getting is LLVM 3.9 specific.

highfive assigned fitzgen Nov 6, 2016

emilio force-pushed the macros branch from 3c2af5e to 2759b05 Compare November 6, 2016 14:28

emilio mentioned this pull request Nov 6, 2016

merge into upstream! #21

Closed

emilio force-pushed the macros branch from 2759b05 to 0976b17 Compare November 6, 2016 15:22

jethrogb reviewed Nov 6, 2016

View reviewed changes

emilio force-pushed the macros branch from 0976b17 to 06991c1 Compare November 6, 2016 17:46

emilio force-pushed the macros branch from 06991c1 to 1ec3363 Compare November 6, 2016 18:07

highfive added the S-needs-rebase label Nov 8, 2016

emilio force-pushed the macros branch from fff088d to 8fa42ef Compare November 8, 2016 05:23

emilio removed the S-needs-rebase label Nov 8, 2016

fitzgen approved these changes Nov 8, 2016

View reviewed changes

emilio added 3 commits November 8, 2016 20:55

Parse macros with cexpr.

e22a11b

Add a finer-grane and more generic API for choosing macro types.

b11e1db

Address review comments.

4ee31ce

emilio force-pushed the macros branch from 8fa42ef to 4ee31ce Compare November 8, 2016 19:59

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 8, 2016

bors-servo pushed a commit that referenced this pull request Nov 9, 2016

Auto merge of #219 - emilio:macros, r=fitzgen

8d83bdf

Parse macro expressions. Clang is trolling me really hard so I'm going to see if the extra token I'm always getting is LLVM 3.9 specific.

bors-servo merged commit 4ee31ce into rust-lang:master Nov 9, 2016

highfive removed the S-awaiting-merge label Nov 9, 2016

Manishearth unassigned fitzgen Jul 25, 2017

Parse macro expressions. #219

Parse macro expressions. #219

Conversation

emilio commented Nov 6, 2016

emilio commented Nov 6, 2016

bors-servo commented Nov 6, 2016

bors-servo commented Nov 6, 2016

emilio commented Nov 6, 2016

bors-servo commented Nov 6, 2016

emilio commented Nov 6, 2016

bors-servo commented Nov 6, 2016

jethrogb Nov 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jethrogb commented Nov 6, 2016

emilio commented Nov 6, 2016

jethrogb commented Nov 6, 2016 • edited Loading

emilio commented Nov 6, 2016

jethrogb commented Nov 6, 2016

emilio commented Nov 6, 2016

jethrogb commented Nov 6, 2016

emilio commented Nov 6, 2016

bors-servo commented Nov 8, 2016

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

jethrogb Nov 6, 2016 •

edited

Loading

jethrogb commented Nov 6, 2016 •

edited

Loading