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

Check for Extra pointer dereferencing #7643

Merged
merged 1 commit into from
Sep 27, 2015

Conversation

jdramani
Copy link
Contributor

Solves issue #7640

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 15, 2015
@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

@jdramani
Copy link
Contributor Author

Build failed as expected.

@jdm
Copy link
Member

jdm commented Sep 16, 2015

@jdramani Want to fix them, too? :)

@jdm
Copy link
Member

jdm commented Sep 16, 2015

Oh, having looked at these changes, I would rather integrate this check into the existing check_rust method, rather than making a new one.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 16, 2015
@Manishearth
Copy link
Member

Doesn't this make more sense as a check_ty lint, since there may be cases when we want to override it?

@jdm
Copy link
Member

jdm commented Sep 16, 2015

@Manishearth: why would we want to override it?

@Manishearth
Copy link
Member

In case of APIs that handle generic container &Ts which don't have ?Sized bounds

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 16, 2015
@nox nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-fails-tidy `./mach test-tidy` reported errors. labels Sep 18, 2015
@nox
Copy link
Contributor

nox commented Sep 18, 2015

./components/compositing/compositor.rs:694: use &[T] instead of &Vec<T>
./components/compositing/compositor_layer.rs:106: use &[T] instead of &Vec<T>
./components/compositing/compositor_layer.rs:286: use &[T] instead of &Vec<T>
./components/gfx/display_list/mod.rs:207: use &[T] instead of &Vec<T>
./components/net/resource_task.rs:64: use &[T] instead of &Vec<T>
./components/net/resource_task.rs:71: use &[T] instead of &Vec<T>
./components/style/animation.rs:710: use &[T] instead of &Vec<T>
./components/style/animation.rs:711: use &[T] instead of &Vec<T>
./components/style/animation.rs:737: use &[T] instead of &Vec<T>
./components/style/animation.rs:738: use &[T] instead of &Vec<T>
./components/style/animation.rs:797: use &[T] instead of &Vec<T>

@jdm jdm removed the S-awaiting-review There is new code that needs to be reviewed. label Sep 18, 2015
@jdm
Copy link
Member

jdm commented Sep 18, 2015

Almost there! Like @nox pointed out, this can't merge until the existing problems this picks up are fixed, too :)
-S-awaiting-review


Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


python/tidy.py, line 310 [r4] (raw file):
This can be if ": &Vec<" in line:.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Sep 18, 2015

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 18, 2015
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #7680) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 18, 2015
@jdm
Copy link
Member

jdm commented Sep 18, 2015

-S-awaiting-review -S-fails-tidy +S-needs-code-changes


Reviewed 5 of 5 files at r5, 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. S-fails-tidy `./mach test-tidy` reported errors. labels Sep 18, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 18, 2015
@Manishearth
Copy link
Member

It looks like you merged master when you should have rebased. Try rebasing over master:

git checkout master
git pull upstream master
git checkout extra_ptr_dref
git rebase master
git push -f origin extra_ptr_deref

@Manishearth
Copy link
Member

Try copying the commands I gave.

@@ -109,8 +109,9 @@ def check_by_line(file_name, contents):
errors = itertools.chain(
check_length(file_name, idx, line),
check_whitespace(idx, line),
check_whatwg_url(idx, line),
check_whatwg_url(idx, line)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the future, you should not remove these commas

@jdramani
Copy link
Contributor Author

Pls take a look at this. Tell me if there is any problem

@jdm
Copy link
Member

jdm commented Sep 27, 2015

Looks great! Thanks for sticking with it :)

@jdm
Copy link
Member

jdm commented Sep 27, 2015

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 2a99915 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Sep 27, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 2a99915 with merge 7413a95...

bors-servo pushed a commit that referenced this pull request Sep 27, 2015
Check for Extra pointer dereferencing

Solves issue #7640

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7643)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 27, 2015
@nox
Copy link
Contributor

nox commented Sep 27, 2015

@bors-servo retry

#7422

@bors-servo
Copy link
Contributor

⌛ Testing commit 2a99915 with merge 9523283...

bors-servo pushed a commit that referenced this pull request Sep 27, 2015
Check for Extra pointer dereferencing

Solves issue #7640

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7643)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Sep 27, 2015
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants