Skip to content

TemplateParameters' methods should just return a Vec, not an Option<Vec> #960

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.

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

Closed
fitzgen opened this issue Sep 7, 2017 · 11 comments
Closed

Comments

@fitzgen
Copy link
Member

fitzgen commented Sep 7, 2017

I thought that the Option would be useful, but in practice we do pretty much ignore that it is an option and do things like params.unwrap_or(vec![]). And then in other places when the result is Some we assume that the contained Vec is not empty, when that isn't always the case.

It will be much simpler to just return a Vec that is either empty or not.

And also usize instead of Option<usize> for num_self_template_params.

The trait's comment will need to be updated as well.

https://github.com/rust-lang-nursery/rust-bindgen/blob/cd41e5cfe26555af574e81b6aa3bed2b84a18000/src/ir/template.rs#L102

@highfive
Copy link

highfive commented Sep 7, 2017

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@fitzgen fitzgen changed the title TemplateParams' methods should just return a Vec, not an Option<Vec> TemplateParameters' methods should just return a Vec, not an Option<Vec> Sep 7, 2017
@hgallagher1993
Copy link

Ya I'll take this @highfive: assign me

@highfive
Copy link

highfive commented Sep 8, 2017

Hey @hgallagher1993! Thanks for your interest in working on this issue. It's now assigned to you!

@fitzgen
Copy link
Member Author

fitzgen commented Sep 8, 2017

@hgallagher1993 let me know if you have any questions!

@fitzgen
Copy link
Member Author

fitzgen commented Sep 25, 2017

@hgallagher1993 making any progress here? Anything I can help with?

@hgallagher1993
Copy link

Yes I have been making progress, There's only a couple compilation errors and then the comments so I should have a pull request in the next day or 2

@hgallagher1993
Copy link

@fitzgen Just as an update this is finished but I'm going to a wedding for the weekend so I'll just make the pull request when I'm back at my laptop on Monday or Tuesday

@fitzgen
Copy link
Member Author

fitzgen commented Sep 29, 2017

Enjoy the wedding :)

ericho added a commit to ericho/rust-bindgen that referenced this issue Feb 4, 2018
The Option<T> returned in this trait was removed to return Vec.

Fixes rust-lang#960.
ericho added a commit to ericho/rust-bindgen that referenced this issue Feb 4, 2018
The Option<T> returned in this trait was removed to return Vec.

Fixes rust-lang#960.
ericho added a commit to ericho/rust-bindgen that referenced this issue Feb 4, 2018
The Option<T> returned in this trait was removed to return Vec.

Fixes rust-lang#960.
@Megamiun
Copy link

Hello, this issue is still open, no?

I can try to solve this, if yes.

@emilio
Copy link
Contributor

emilio commented Feb 17, 2018

There's #1245, which should be pretty close to be landable.

@Megamiun
Copy link

Oh, I confounded this PR with the other. Hope it goes alright with this PR!

ericho added a commit to ericho/rust-bindgen that referenced this issue Feb 19, 2018
The Option<T> returned in this trait was removed to return Vec.

Fixes rust-lang#960.
bors-servo pushed a commit that referenced this issue Apr 8, 2018
TemplateParameters do not return Option

Fixes #960.
Closes #1245.

I found it useful to do this incrementally, changing each method in a separate commit and ensuring the tests continue to pass unchanged.

r? @emilio
/cc @ericho
bors-servo pushed a commit that referenced this issue Apr 10, 2018
TemplateParameters do not return Option

Fixes #960.
Closes #1245.

I found it useful to do this incrementally, changing each method in a separate commit and ensuring the tests continue to pass unchanged.

r? @emilio
/cc @ericho
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants