Wrap enum_val_signed in an Option. #220

jcdyer · 2016-11-06T17:00:52Z

Also reorganize calling code to avoid duplicate checking of cursor type.

Fixes #127

highfive · 2016-11-06T17:00:54Z

Warning

These commits modify unsafe code. Please review it carefully!

nox · 2016-11-06T17:02:07Z

src/clang.rs

@@ -352,8 +352,14 @@ impl Cursor {
    /// Returns `LLONG_MIN` if the cursor's referent is not an enum variant,
    /// which is also a valid enum value, so callers should check the cursor
    /// kind before calling this method (see issue #127).


That comment is now obsolete.

So it is. Fixed.

emilio

Looks good to me with those changes, thanks for doing this! :)

emilio · 2016-11-06T17:16:47Z

src/ir/enum_ty.rs

@@ -71,16 +71,20 @@ impl Enum {
        };

        declaration.visit(|cursor| {
-            if cursor.kind() == CXCursor_EnumConstantDecl {


why not keeping the if above as before, and then just unwrap the result of enum_val_signed?

I like to avoid unwrapping when it's easy to do, because it makes the code here less intertwined with code elsewhere; if enum_val_signed changes to return None for some other reason, that's a future bug avoided. I also think once #128 lands, it'll look nice and clean.

I'm pretty new at rust though, and brand new to rust-bindgen, so if I'm missing some bigger picture reason why it's worth keeping the duplicate check, I'm happy to change it.

I sort of agree, and in general it uses to be good advice. But in this case, IMO at least, this is inverting the dependency instead.

That is, if for some other reason enum_val_signed returns Some when it's not a CXCursor_EnumConstantDecl, the code meaning is completely different, and we're in trouble.

We're traversing the ast looking for EnumConstantDecl nodes, so I think that if should stay there instead of relying that this only returns Some if that's the case.

Makes sense. Here's a new version with the outer if, but still without unwrap(), for the reason above. If I understand rust correctly, there shouldn't be any runtime time penalty, right?

I don't think there's a runtime penalty for using unwrap vs if let. Maybe rust annotates unwrap as unlikely so it has a bit better branch prediction, but nothing measurable for this case I think :)

emilio · 2016-11-06T17:17:40Z

src/ir/enum_ty.rs

@@ -71,16 +71,20 @@ impl Enum {
        };

        declaration.visit(|cursor| {
-            if cursor.kind() == CXCursor_EnumConstantDecl {
+            let val = if is_signed {
+                cursor.enum_val_signed().map(|val| EnumVariantValue::Signed(val))


Just for reference, you can do cursor.enum_val_signed().map(EnumVariantValue::Signed) :)

Makes sense. Updated.

emilio · 2016-11-06T17:18:10Z

src/ir/enum_ty.rs

                variants.push(EnumVariant::new(name, comment, val));
-            }
+            };


This semicolon shouldn't be needed.

Also reorganize calling function to avoid duplicate checking of cursor type. Fixes rust-lang#127

jcdyer · 2016-11-06T17:36:47Z

Squashed commits.

KiChjang · 2016-11-06T17:53:17Z

src/ir/enum_ty.rs

+            let val = if is_signed {
+                cursor.enum_val_signed().map(EnumVariantValue::Signed)
+            } else {
+                if cursor.kind() == CXCursor_EnumConstantDecl {


This looks like too much rightward drift. Why not meld this into the previous level and use else if?

I wanted to keep it visually separate, because the inner if/else block can go away after #128 lands.

emilio · 2016-11-06T21:56:21Z

@bors-servo r+

Thanks! :)

bors-servo · 2016-11-06T21:56:22Z

📌 Commit fda0548 has been approved by emilio

bors-servo · 2016-11-06T21:56:23Z

⌛ Testing commit fda0548 with merge b47f405...

Wrap enum_val_signed in an Option. Also reorganize calling code to avoid duplicate checking of cursor type. Fixes #127

bors-servo · 2016-11-06T21:56:30Z

💡 This pull request was already approved, no need to approve it again.

This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

bors-servo · 2016-11-06T21:56:31Z

📌 Commit fda0548 has been approved by emilio

bors-servo · 2016-11-06T22:21:37Z

☀️ Test successful - status-travis

Wrap enum_val_unsigned in an Option Patterned after the changes merged in #220. Fixes #128

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

nox reviewed Nov 6, 2016

View reviewed changes

emilio approved these changes Nov 6, 2016

View reviewed changes

Wrap enum_val_signed in an Option.

15d1881

Also reorganize calling function to avoid duplicate checking of cursor type. Fixes rust-lang#127

jcdyer mentioned this pull request Nov 6, 2016

clang::Cursor::enum_val_unsigned should return Option<u64> #128

Closed

KiChjang reviewed Nov 6, 2016

View reviewed changes

emilio added S-needs-code-changes and removed S-awaiting-review labels Nov 6, 2016

Restore outer conditional.

fda0548

jcdyer force-pushed the enum_value_signed branch from 0bdc4de to fda0548 Compare November 6, 2016 21:52

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 6, 2016

highfive assigned emilio Nov 6, 2016

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

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

Auto merge of #220 - jcdyer:enum_value_signed, r=emilio

b47f405

Wrap enum_val_signed in an Option. Also reorganize calling code to avoid duplicate checking of cursor type. Fixes #127

bors-servo merged commit fda0548 into rust-lang:master Nov 6, 2016

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

afluth mentioned this pull request Nov 7, 2016

Wrap enum_val_unsigned in an Option #222

Merged

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

Auto merge of #222 - afluth:enum_val_unsigned_option, r=emilio

c314a2f

Wrap enum_val_unsigned in an Option Patterned after the changes merged in #220. Fixes #128

Wrap enum_val_signed in an Option. #220

Wrap enum_val_signed in an Option. #220

Uh oh!

Conversation

jcdyer commented Nov 6, 2016

Uh oh!

highfive commented Nov 6, 2016

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!

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!

jcdyer Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcdyer commented Nov 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors-servo commented Nov 6, 2016

Uh oh!

bors-servo commented Nov 6, 2016

Uh oh!

bors-servo commented Nov 6, 2016

Uh oh!

bors-servo commented Nov 6, 2016

Uh oh!

bors-servo commented Nov 6, 2016

Uh oh!

Uh oh!

jcdyer Nov 6, 2016 •

edited

Loading

emilio commented Nov 6, 2016 •

edited

Loading