Skip to content

Conversation

@ekse
Copy link
Contributor

@ekse ekse commented Nov 10, 2018

Issue: #428

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Looks ok, but needs tests, any chance you can add some basic tests to bindgen-integration?

@jethrogb
Copy link
Contributor

lgtm

@ekse
Copy link
Contributor Author

ekse commented Nov 11, 2018

I added two tests, let me know if you need more.

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Looks fine, thanks! It may also be worth adding tests with types and users of the test, so something like:

struct my_prefix_bar {
    int foo;
};

struct my_prefix_foo {
   my_prefix_bar member;
};

Just knowing it builds is probably fine :)

@ekse
Copy link
Contributor Author

ekse commented Nov 11, 2018

I added the test you mentioned, I create an instance of the foo struct to validate it works as expected.

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@emilio
Copy link
Contributor

emilio commented Nov 11, 2018

@bors-servo r+

@bors-servo
Copy link

📌 Commit 5b741da has been approved by emilio

@bors-servo
Copy link

⌛ Testing commit 5b741da with merge 8489d42...

bors-servo pushed a commit that referenced this pull request Nov 11, 2018
Add item_name parse callback.

Issue: #428
@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: emilio
Pushing 8489d42 to master...

@bors-servo bors-servo merged commit 5b741da into rust-lang:master Nov 11, 2018
@ekse ekse deleted the item_name branch November 11, 2018 02:10
@emilio emilio mentioned this pull request Nov 11, 2018
@jethrogb
Copy link
Contributor

jethrogb commented Nov 28, 2018

This turns out to be not very useful, because whitelist/blacklist checking is done after the mapping in the PR is performed. @emilio do you know what the correct place to put the rename logic would be?

@emilio
Copy link
Contributor

emilio commented Nov 28, 2018

This turns out to be not very useful, because whitelist/blacklist checking is done after the mapping in the PR is performed. @emilio do you know what the correct place to put the rename logic would be?

Well that means that you could whitelist with the stripped prefix, right? I'd rather not introduce two different names... The nice thing of this PR is that the complexity doesn't blow up because everything uses the new name.

But we could somehow add a new option NameOptions to avoid using the callback, and somehow use that for whitelisting / blacklisting.

@jethrogb
Copy link
Contributor

Well that means that you could whitelist with the stripped prefix, right?

That's not possible, because the whitelist regex is matching exactly on the part that's being stripped.

@emilio
Copy link
Contributor

emilio commented Nov 28, 2018

Oh I see, that's indeed unfortunate... Let me think a bit about how to do this.

@emilio
Copy link
Contributor

emilio commented Nov 28, 2018

I opened #1452, could you give that a spin?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants