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
Merged

Conversation

@jdramani
Copy link
Contributor

jdramani commented Sep 15, 2015

Solves issue #7640

Review on Reviewable

@highfive
Copy link

highfive commented Sep 15, 2015

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

jdramani commented Sep 15, 2015

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.

@Manishearth
Copy link
Member

Manishearth commented Sep 16, 2015

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

Manishearth commented Sep 16, 2015

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

@nox
Copy link
Member

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 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

@bors-servo
Copy link
Contributor

bors-servo commented Sep 18, 2015

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

@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

@Manishearth
Copy link
Member

Manishearth commented Sep 19, 2015

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

Manishearth commented Sep 26, 2015

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)

This comment has been minimized.

@frewsxcv

frewsxcv Sep 26, 2015

Member

In the future, you should not remove these commas

@jdramani jdramani force-pushed the jdramani:extra_ptr_dref branch from 8a836fb to 66c1a6f Sep 27, 2015
jdramani added a commit to jdramani/servo that referenced this pull request Sep 27, 2015
@jdramani jdramani force-pushed the jdramani:extra_ptr_dref branch from 66c1a6f to 9b1d234 Sep 27, 2015
jdramani added a commit to jdramani/servo that referenced this pull request Sep 27, 2015
@jdramani jdramani force-pushed the jdramani:extra_ptr_dref branch from 9b1d234 to 2a99915 Sep 27, 2015
@jdramani
Copy link
Contributor Author

jdramani commented Sep 27, 2015

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
Copy link
Contributor

bors-servo commented Sep 27, 2015

📌 Commit 2a99915 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2015

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

bors-servo commented Sep 27, 2015

💔 Test failed - mac-rel-wpt

@nox
Copy link
Member

nox commented Sep 27, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2015

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 -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 27, 2015

@bors-servo bors-servo merged commit 2a99915 into servo:master Sep 27, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jdramani jdramani deleted the jdramani:extra_ptr_dref branch Sep 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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