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

Update Rust and use the newly-stable std::ptr::NonNull #19829

Merged
merged 6 commits into from Jan 22, 2018
Merged

Update Rust and use the newly-stable std::ptr::NonNull #19829

merged 6 commits into from Jan 22, 2018

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Jan 22, 2018

This change is Reviewable

@highfive
Copy link

highfive commented Jan 22, 2018

Heads up! This PR modifies the following files:

  • @bholley: ports/geckolib/glue.rs
  • @asajeffrey: components/script/dom/bindings/nonnull.rs, components/script/dom/textencoder.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/screen.rs, components/script/dom/crypto.rs and 23 more
  • @KiChjang: components/script/dom/bindings/nonnull.rs, components/script/dom/textencoder.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/screen.rs, components/script/dom/crypto.rs and 23 more
  • @fitzgen: components/script/dom/bindings/nonnull.rs, components/script/dom/textencoder.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/screen.rs, components/script/dom/crypto.rs and 23 more
  • @emilio: ports/geckolib/glue.rs, components/layout/model.rs, components/layout/wrapper.rs, components/layout/query.rs
@highfive
Copy link

highfive commented Jan 22, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@nox
nox approved these changes Jan 22, 2018
Copy link
Member

nox left a comment

Looks good to me, feel free to handle my remarks as a follow-up issue. Would be nice if you filed an issue about auditing all the new_unchecked calls in general, some of them scare me.

@@ -106,7 +106,7 @@ impl<T: DomObject + JSTraceable + Iterable> IterableIterator<T> {
self.index.set(index + 1);
result.map(|_| {
assert!(!rval.is_null());
unsafe { NonNullJSObjectPtr::new_unchecked(rval.get()) }
unsafe { NonNull::new_unchecked(rval.get()) }

This comment has been minimized.

Copy link
@nox

nox Jan 22, 2018

Member

Use new and expect.

assert!(!self.data.get().is_null());
NonNullJSObjectPtr::new_unchecked(self.data.get())
NonNull::new_unchecked(self.data.get())

This comment has been minimized.

Copy link
@nox

nox Jan 22, 2018

Member

Use new and expect.

rooted!(in(cx) let array = JS_NewUint8ClampedArray(cx, 16));
assert!(!array.is_null());
NonNullJSObjectPtr::new_unchecked(array.get())
NonNull::new_unchecked(array.get())

This comment has been minimized.

Copy link
@nox

nox Jan 22, 2018

Member

Use new and expect.

// Step 1
let created = self.response_arraybuffer.get();
if !created.is_null() {
return Some(NonNullJSObjectPtr::new_unchecked(created));
return Some(NonNull::new_unchecked(created));

This comment has been minimized.

Copy link
@nox

nox Jan 22, 2018

Member

Use new.

@SimonSapin SimonSapin force-pushed the rustup branch from 339869c to 9fbf006 Jan 22, 2018
@nox
Copy link
Member

nox commented Jan 22, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

📌 Commit 9fbf006 has been approved by nox

@highfive highfive assigned nox and unassigned KiChjang Jan 22, 2018
@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

Testing commit 9fbf006 with merge 0d3458c...

bors-servo added a commit that referenced this pull request Jan 22, 2018
Update Rust and use the newly-stable std::ptr::NonNull

<!-- 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/19829)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

💔 Test failed - linux-dev

@CYBAI
Copy link
Collaborator

CYBAI commented Jan 22, 2018

   Compiling libservo v0.0.1 (file:///home/servo/buildbot/slave/linux-dev/build/components/servo)
   Compiling glutin_app v0.0.1 (file:///home/servo/buildbot/slave/linux-dev/build/ports/glutin)
   Compiling embedding v0.0.1 (file:///home/servo/buildbot/slave/linux-dev/build/ports/cef)
error: unnecessary parentheses around function argument
  --> ports/cef/eutil.rs:33:34
   |
33 |     let object = libc::calloc(1, (mem::size_of::<Base>() + mem::size_of::<Extra>())) as
   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses
   |
   = note: `-D unused-parens` implied by `-D warnings`

error: unnecessary parentheses around function argument
   --> ports/cef/wrappers.rs:202:36
    |
202 |             let ptr = libc::malloc(((buffer.len() + 1) * 2)) as *mut c_ushort;
    |                                    ^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these parentheses

error: aborting due to 2 previous errors

error: Could not compile `embedding`.
@SimonSapin
Copy link
Member Author

SimonSapin commented Jan 22, 2018

@bors-servo r=nox

@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

📌 Commit 4be3096 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

Testing commit 4be3096 with merge c1ed4bb...

bors-servo added a commit that referenced this pull request Jan 22, 2018
Update Rust and use the newly-stable std::ptr::NonNull

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

jdm commented Jan 22, 2018

  ▶ TIMEOUT [expected PASS] /css/CSS2/box-display/containing-block-009.xht
  │ 
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 17.3.0-devel
  │ 2018-01-22 10:30:57.066 servo[81885:285780639] Metadata.framework [Error]: couldn't get the client port
  │ Stack trace for thread "WRRenderBackend#0"
  │ stack backtrace:
  │    0:        0x10f8f3aee - backtrace::backtrace::trace::h1572cbde15e97645
  │    1:        0x10f8f42dc - backtrace::capture::Backtrace::new::h0ca27cd1e659fd9b
  │    2:        0x10d3219a0 - servo::install_crash_handler::handler::hef6d72ec40496654
  │    3:     0x7fff8f88ff19 - _sigtramp
  │    4:        0x10ef1a4c7 - _$LT$std..collections..hash..map..HashMap$LT$K$C$$u20$V$C$$u20$S$GT$$GT$::contains_key::h6f7b7a7f90f60aa7
  │    5:        0x10eed3f15 - webrender::glyph_rasterizer::_$LT$impl$u20$webrender..platform..macos..font..FontContext$GT$::add_font::h855f3dfb504b8fe4
  │    6:        0x10ef64457 - webrender::glyph_rasterizer::GlyphRasterizer::add_font::h8f362be6e920367e
  │    7:        0x10eefea16 - webrender::resource_cache::ResourceCache::add_font_template::hd9fa88ab61ef0758
  │    8:        0x10eefd306 - webrender::resource_cache::ResourceCache::update_resources::h4a4d139987a02082
  │    9:        0x10ef4d9c9 - webrender::render_backend::RenderBackend::run::h3fba174366c9d443
  │   10:        0x10ee879e1 - std::sys_common::backtrace::__rust_begin_short_backtrace::h3370e1c89a690f63
  │   11:        0x10eeb9b1d - std::panicking::try::do_call::h939a1adbd2b681c9 (.llvm.BA14D8D4)
  │   12:        0x10f923fae - __rust_maybe_catch_panic
  │   13:        0x10ef59f93 - _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::heff2fe36abcdc229
  │   14:        0x10f90cd67 - std::sys_common::thread::start_thread::h1ef8cf6b973abe77
  │   15:        0x10f9100e8 - std::sys::unix::thread::Thread::new::thread_start::h88d15e6243c319fe
  │   16:     0x7fff91ccf059 - _pthread_body
  │   17:     0x7fff91ccefd6 - _pthread_start

Terrifying!

@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

@bors-servo bors-servo merged commit 4be3096 into master Jan 22, 2018
2 of 5 checks passed
2 of 5 checks passed
Taskcluster (pull_request) TaskGroup: failure
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details
@SimonSapin SimonSapin deleted the rustup branch Jan 23, 2018
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.