Skip to content

/ servo

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

deny warnings for all crates as default #19573

Closed
wants to merge 1 commit into from
Closed

deny warnings for all crates as default #19573

wants to merge 1 commit into from

Conversation

@tigercosmos
Copy link
Collaborator

tigercosmos commented Dec 15, 2017 •
edited by emilio

deny warnings for all crates as default
r? emilio

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #19572 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@highfive
Copy link

highfive commented Dec 15, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/servo_arc/lib.rs, components/selectors/lib.rs
  • @jgraham: components/webdriver_server/lib.rs
  • @emilio: components/layout/lib.rs
  • @fitzgen: components/script_plugins/lib.rs, components/debugger/lib.rs, components/script/lib.rs, components/script_traits/lib.rs, components/script_traits/lib.rs and 7 more
  • @KiChjang: components/net/lib.rs, components/script_plugins/lib.rs, components/script/lib.rs, components/net_traits/lib.rs, components/net_traits/lib.rs and 3 more
  • @asajeffrey: components/script_plugins/lib.rs, components/constellation/lib.rs, components/script/lib.rs, components/script_traits/lib.rs, components/script_layout_interface/lib.rs and 1 more
  • @cbrewster: components/constellation/lib.rs
  • @paulrouget: components/compositing/lib.rs, components/constellation/lib.rs, components/servo/lib.rs, components/servo_arc/lib.rs
@highfive
Copy link

highfive commented Dec 15, 2017

warning Warning warning

  • These commits modify net, layout, gfx, and script code, but no tests are modified. Please consider adding a test!
@emilio
emilio approved these changes Dec 15, 2017
View changes
Copy link
Member

emilio left a comment

Fine for me, thanks!

@jdm or anyone else, any opinion against this?
@jdm
Copy link
Member

jdm commented Dec 15, 2017

Historically blanket denies like this can make local debugging more frustrating, since attempts to comment out pieces of code can make arguments or fictions unused. I'm not sure that the benefit of not merging unused imports is worth it.
@bors-servo
Copy link
Contributor

bors-servo commented Dec 15, 2017

☔ The latest upstream changes (presumably #19578) made this pull request unmergeable. Please resolve the merge conflicts.
@KiChjang
Copy link
Member

KiChjang commented Dec 15, 2017

Moreover, we're still using the nightly channel, so any erroneous warning messages from nightly could cause some roadblocks when we're performing a rustup.
@tigercosmos
Copy link
Collaborator Author

tigercosmos commented Dec 16, 2017 •
edited

I thought the question is which one will be better:

  1. deny warning as default, and modify if need (nightly or ..?)
  2. when find warning, fix it manually

Or maybe not "all" set as default, but "almost"?
@SimonSapin
Copy link
Member

SimonSapin commented Dec 18, 2017

@KiChjang I don’t remember ever seeing erroneous warnings so far. Have you? Which ones?
@KiChjang
Copy link
Member

KiChjang commented Dec 18, 2017

I haven't, but I suppose that's not quite important since we can just use a different nightly version...
@tigercosmos
Copy link
Collaborator Author

tigercosmos commented Dec 19, 2017

How to determine let the PR pass or not?
I don't mind to close this one if most members deny this.
@tigercosmos
deny warnings for all crates as default
Loading status checks…
ab53c3c
This was referenced Dec 20, 2017
Merged
Merged
Merged
@jdm
Copy link
Member

jdm commented Dec 20, 2017

I haven't seen much enthusiasm for denying warnings from other people, and I still feel strongly that it makes debugging much more frustrating. I'm going to go ahead and close this.
@jdm jdm closed this Dec 20, 2017
@tigercosmos tigercosmos deleted the tigercosmos:v1 branch Dec 20, 2017
bors-servo added a commit that referenced this pull request Dec 20, 2017
@bors-servo
Auto merge of #19609 - tigercosmos:q1, r=emilio
Loading status checks…
8f786d4 
remove unsued IpcReceiver

<!-- Please describe your changes on the following line: -->
```
warning: unused import: `IpcReceiver`
  --> components/canvas/canvas_paint_thread.rs:14:41
   |
14 | use ipc_channel::ipc::{self, IpcSender, IpcReceiver};
   |                                         ^^^^^^^^^^^
   |
   = note: #[warn(unused_imports)] on by default
```

recently I have done similar commits for three times
Shall we merge #19573?

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

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

<!-- 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/19609)
<!-- Reviewable:end -->
@SimonSapin
Copy link
Member

SimonSapin commented Dec 20, 2017 •
edited

I haven't seen much enthusiasm

I only clicked the "thumb up" button without commenting because I didn’t realize I should be more vocal, but I’m rather in favor of doing something more to not land PRs that introduce warnings.

Warnings unrelated to what you’re working on at the moment are noise that you need to visually filter out from relevant output at every recompilation. And sending "Fix warnings" PRs is busy work.

@jdm, what do you think of setting RUSTFLAGS="-D warnings" on CI? That way debugging is not affected.
@jdm
Copy link
Member

jdm commented Dec 20, 2017

That sounds like a perfectly fine choice to me!
@tigercosmos
Copy link
Collaborator Author

tigercosmos commented Dec 20, 2017

@SimonSapin What CI files should be included?
travis, buildbot?
@jdm
Copy link
Member

jdm commented Dec 20, 2017

You will want to modify travisCI, Appveyor, and buildbot_steps.yml.
@tigercosmos tigercosmos mentioned this pull request Dec 20, 2017
Merged
0 of 5 tasks complete
bors-servo added a commit that referenced this pull request Dec 20, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=SimonSapin
Loading status checks…
a73bd82 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 20, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=<try>
Loading status checks…
b8b9b31 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 20, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=<try>
Loading status checks…
4695789 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 21, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=<try>
Loading status checks…
1c6201c 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 21, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=SimonSapin,jdm,emilio
Loading status checks…
206abb3 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 21, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=SimonSapin,jdm,emilio
Loading status checks…
f35a457 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 21, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=<try>
Loading status checks…
5b85b85 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 21, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=<try>
Loading status checks…
cf1d6a9 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 21, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=<try>
Loading status checks…
9124aa9 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 21, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=<try>
Loading status checks…
4038f89 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 21, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=<try>
Loading status checks…
82c2368 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Dec 21, 2017
@bors-servo
Auto merge of #19612 - tigercosmos:o1, r=jdm
Loading status checks…
2eb1512 
deny warnings

<!-- Please describe your changes on the following line: -->
deny warnings
related to #19573

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

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

<!-- 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/19612)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
S-awaiting-review S-needs-rebase
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

check build warning about unused import
7 participants
@tigercosmos @highfive @jdm @bors-servo @KiChjang @SimonSapin @emilio
You can’t perform that action at this time.