Skip to content
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

Move `contain` outside of mako. #19656

Closed
emilio opened this issue Dec 30, 2017 · 5 comments
Closed

Move `contain` outside of mako. #19656

emilio opened this issue Dec 30, 2017 · 5 comments

Comments

@emilio
Copy link
Member

@emilio emilio commented Dec 30, 2017

See #19015.

@emilio emilio mentioned this issue Dec 30, 2017
48 of 49 tasks complete
@emilio
Copy link
Member Author

@emilio emilio commented Dec 30, 2017

Tentatively assigning it to @stevel98 :)

@emilio emilio added the C-assigned label Dec 30, 2017
@stevel98
Copy link
Contributor

@stevel98 stevel98 commented Dec 31, 2017

Hi,
I'm still new to rust, and I'm puzzled over the purpose of the computed_value module under the contains longhand in box.mako.rs.

pub mod computed_value {
    pub type T = super::SpecifiedValue;
}

I only see it used in:

#[inline]
pub fn get_initial_value() -> computed_value::T {
    computed_value::T::empty()
}

But wouldn't this be the same as:

#[inline]
pub fn get_initial_value() -> computed_value::T {
    Self::empty()
}
@CYBAI
Copy link
Collaborator

@CYBAI CYBAI commented Dec 31, 2017

@stevel98

This block means you can just implement the Contain type inside specified module; then, due to same for both specified and computed, you can just export (pub use) the specified type in computed module. You can check this PR. I think they're not same but similar.

pub mod computed_value {
    pub type T = super::SpecifiedValue;
}

For this block, yeah, you can use Self::empty().

#[inline]
pub fn get_initial_value() -> computed_value::T {
    computed_value::T::empty()
}

Another note:

For this case, I didn't try but maybe you don't need this get_initial_value function? I think you can just use empty for initial_value?

cc @emilio
If I'm wrong, please correct me :P

@emilio
Copy link
Member Author

@emilio emilio commented Dec 31, 2017

Yes, but in this case, since the types are the same, it's completely redundant to use computed_value::T instead of Self. This is the kind of stuff we're trying to remove :)

@stevel98
Copy link
Contributor

@stevel98 stevel98 commented Dec 31, 2017

Thanks! I'll try to remove it then.

@stevel98 stevel98 mentioned this issue Jan 4, 2018
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Jan 4, 2018
style: Move contain outside of mako

<!-- Please describe your changes on the following line: -->
Sub-PR for #19015.
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #19656 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19682)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 4, 2018
style: Move contain outside of mako

<!-- Please describe your changes on the following line: -->
Sub-PR for #19015.
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #19656 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19682)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.