Fragment debug_id u16 only exists in debug, prod will format mem address #11442

mitchhentges · 2016-05-26T16:38:51Z

Each fragment has a u16 debug_id in debug mode, but no debug_id in production to save memory. To format a debug id in production, the address of the empty debug_id is displayed.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #87 (github issue number if applicable).

These changes do not require tests because it looks like it's not possible to mock out cfg options in #[test]s

This change is

highfive · 2016-05-26T16:38:56Z

Warning

These commits modify layout code, but no tests are modified. Please consider adding a test!

mitchhentges · 2016-05-26T16:39:20Z

CI is mad right out-of-the-gate :(

highfive · 2016-05-27T16:55:34Z

New code was committed to pull request.

mitchhentges · 2016-05-27T16:56:22Z

Merged master to make CI go again (Appveyor had failed immediately for the first commit)

jdm · 2016-05-27T18:35:22Z

If appveyor shows a build error unrelated to your changes, it's not a big deal.

KiChjang · 2016-05-31T06:32:18Z

components/layout/fragment.rs

+
+#[cfg(debug_assertions)]
+#[derive(Clone)]
+struct DebugId {


This could just be struct DebugId(u16).

highfive · 2016-05-31T08:56:07Z

New code was committed to pull request.

mitchhentges · 2016-05-31T08:56:31Z

I also made the not(debug_assertions) one a "unit-like" struct

KiChjang · 2016-05-31T14:53:24Z

@bors-servo r+

Thanks!

bors-servo · 2016-05-31T14:53:26Z

📌 Commit 14a90d0 has been approved by KiChjang

bors-servo · 2016-06-01T04:15:16Z

⌛ Testing commit 14a90d0 with merge 170a21e...


        Auto merge of #11442 - mitchhentges:87-debug-id, r=KiChjang

Fragment debug_id u16 only exists in debug, prod will format mem address  Each fragment has a `u16` `debug_id` in debug mode, but no `debug_id` in production to save memory. To format a debug id in production, the address of the empty `debug_id` is displayed. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #87 (github issue number if applicable).  - [X] These changes do not require tests because it looks like it's not possible to mock out `cfg` options in `#[test]`s   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11442)

bors-servo · 2016-06-01T04:27:22Z

💔 Test failed - linux-rel

KiChjang · 2016-06-01T04:43:27Z

Fails compiling release:

/home/servo/buildbot/slave/linux-rel/build/components/layout/fragment.rs:2819:31: 2819:37 error: invalid reference to argument `0` (no arguments given)
/home/servo/buildbot/slave/linux-rel/build/components/layout/fragment.rs:2819         e.emit_string(format!("{:p}"), &self)
                                                                                                            ^~~~~~
<std macros>:2:28: 2:59 note: in this expansion of format_args!
/home/servo/buildbot/slave/linux-rel/build/components/layout/fragment.rs:2819:23: 2819:38 note: in this expansion of format! (defined in <std macros>)
error: aborting due to previous error
error: Could not compile `layout`.

highfive · 2016-06-01T11:49:28Z

New code was committed to pull request.

mitchhentges · 2016-06-01T11:51:20Z

Ah, you do ./mach build --release to build without debug_assertions to test the other compile path, cool.
Fixed, and I also added some #[cfg(not(debug_assertions))] to try and not create DEBUG_ID_COUNTER, generate_unique_debug_id() and the import for layout_debug, but I'm still getting warnings in the console. Maybe I should ignore the "dead code" warning?

/home/mitch/dev/servo/components/layout/layout_debug.rs:23:1: 23:58 warning: static item is never used: `DEBUG_ID_COUNTER`, #[warn(dead_code)] on by default
/home/mitch/dev/servo/components/layout/layout_debug.rs:23 static DEBUG_ID_COUNTER: AtomicUsize = ATOMIC_USIZE_INIT;
                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/mitch/dev/servo/components/layout/layout_debug.rs:101:1: 103:2 warning: function is never used: `generate_unique_debug_id`, #[warn(dead_code)] on by default
/home/mitch/dev/servo/components/layout/layout_debug.rs:101 pub fn generate_unique_debug_id() -> u16 {
                                                            ^
/home/mitch/dev/servo/components/layout/fragment.rs:53:5: 53:17 warning: unused import, #[warn(unused_imports)] on by default
/home/mitch/dev/servo/components/layout/fragment.rs:53 use layout_debug;

highfive · 2016-06-04T08:49:14Z

New code was committed to pull request.

mitchhentges · 2016-06-04T08:50:31Z

Okay. I think I've got this covered. I've:

Run ./mach test-tidy
Run ./mach build -d
Run ./mach build --release

I don't know what else I can do other than run all tests locally, which will take a little too long. So, I'm pretty confident that it should pass CI now 😄

KiChjang · 2016-06-04T16:50:56Z

Fails tidy:

./components/layout/fragment.rs:52: encountered whitespace following a use statement

./components/layout/layout_debug.rs:19: encountered whitespace following a use statement

mitchhentges · 2016-06-04T17:18:07Z

I must've made a mistake on my git add, agh. Handling this outside my ide (for amends) is throwing me off


        Fragment debug_id u16 only exists in debug, prod will format mem address

highfive · 2016-06-04T17:19:57Z

New code was committed to pull request.

KiChjang · 2016-06-04T18:12:04Z

@bors-servo r+

bors-servo · 2016-06-04T18:12:05Z

📌 Commit 43396c0 has been approved by KiChjang

bors-servo · 2016-06-04T18:21:33Z

⌛ Testing commit 43396c0 with merge a149d75...


        Auto merge of #11442 - mitchhentges:87-debug-id, r=KiChjang

Fragment debug_id u16 only exists in debug, prod will format mem address  Each fragment has a `u16` `debug_id` in debug mode, but no `debug_id` in production to save memory. To format a debug id in production, the address of the empty `debug_id` is displayed. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #87 (github issue number if applicable).  - [X] These changes do not require tests because it looks like it's not possible to mock out `cfg` options in `#[test]`s   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11442)

bors-servo · 2016-06-04T18:41:42Z

💔 Test failed - mac-rel-css

mitchhentges · 2016-06-04T20:26:15Z

I don't think that the failing build is due to my changes:


command timed out: 1200 seconds without output running ['git', 'clean', '-f', '-f', '-d', '-x'], attempting to kill

KiChjang · 2016-06-04T20:36:55Z

@bors-servo retry

The annoying git


        Auto merge of #11442 - mitchhentges:87-debug-id, r=KiChjang

Fragment debug_id u16 only exists in debug, prod will format mem address  Each fragment has a `u16` `debug_id` in debug mode, but no `debug_id` in production to save memory. To format a debug id in production, the address of the empty `debug_id` is displayed. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #87 (github issue number if applicable).  - [X] These changes do not require tests because it looks like it's not possible to mock out `cfg` options in `#[test]`s   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11442)

bors-servo · 2016-06-04T21:04:00Z

⌛ Testing commit 43396c0 with merge 5002dff...

bors-servo · 2016-06-04T21:53:50Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned KiChjang May 26, 2016

mitchhentges force-pushed the mitchhentges:87-debug-id branch from dce044b to 9e224f6 May 27, 2016

KiChjang reviewed May 31, 2016
View changes

components/layout/fragment.rs Outdated

#[cfg(debug_assertions)]

#[derive(Clone)]

struct DebugId {

This comment has been minimized.

Sign in to view

Copy link

KiChjang May 31, 2016
Member

This could just be struct DebugId(u16).

KiChjang added S-needs-code-changes and removed S-awaiting-review labels May 31, 2016

mitchhentges force-pushed the mitchhentges:87-debug-id branch from 9e224f6 to 14a90d0 May 31, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels May 31, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 31, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 1, 2016

mitchhentges force-pushed the mitchhentges:87-debug-id branch from 14a90d0 to 971743d Jun 1, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jun 1, 2016

mitchhentges force-pushed the mitchhentges:87-debug-id branch from a864cad to b65643c Jun 4, 2016

KiChjang removed the S-awaiting-review label Jun 4, 2016

Fragment debug_id u16 only exists in debug, prod will format mem address

Loading status checks…

43396c0

mitchhentges force-pushed the mitchhentges:87-debug-id branch from b65643c to 43396c0 Jun 4, 2016

highfive added the S-awaiting-review label Jun 4, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 4, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 4, 2016

KiChjang removed the S-fails-tidy label Jun 4, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Jun 4, 2016

bors-servo merged commit 43396c0 into servo:master Jun 4, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 4, 2016

mitchhentges deleted the mitchhentges:87-debug-id branch Jun 5, 2016

servo / servo

Fragment debug_id u16 only exists in debug, prod will format mem address #11442

Fragment debug_id u16 only exists in debug, prod will format mem address #11442

mitchhentges commented May 26, 2016 •

edited by jdm

highfive commented May 26, 2016

mitchhentges commented May 26, 2016

highfive commented May 27, 2016

mitchhentges commented May 27, 2016

jdm commented May 27, 2016

This comment has been minimized.

highfive commented May 31, 2016

mitchhentges commented May 31, 2016

KiChjang commented May 31, 2016

bors-servo commented May 31, 2016

bors-servo commented Jun 1, 2016

bors-servo commented Jun 1, 2016

KiChjang commented Jun 1, 2016

highfive commented Jun 1, 2016

mitchhentges commented Jun 1, 2016

highfive commented Jun 4, 2016

mitchhentges commented Jun 4, 2016

KiChjang commented Jun 4, 2016

mitchhentges commented Jun 4, 2016

highfive commented Jun 4, 2016

KiChjang commented Jun 4, 2016

bors-servo commented Jun 4, 2016

bors-servo commented Jun 4, 2016

bors-servo commented Jun 4, 2016

mitchhentges commented Jun 4, 2016 •

edited

KiChjang commented Jun 4, 2016

bors-servo commented Jun 4, 2016

bors-servo commented Jun 4, 2016

servo / servo

Join GitHub today

Fragment debug_id u16 only exists in debug, prod will format mem address #11442

Fragment debug_id u16 only exists in debug, prod will format mem address #11442

Conversation

mitchhentges commented May 26, 2016 • edited by jdm

highfive commented May 26, 2016

mitchhentges commented May 26, 2016

highfive commented May 27, 2016

mitchhentges commented May 27, 2016

jdm commented May 27, 2016

This comment has been minimized.

KiChjang May 31, 2016

highfive commented May 31, 2016

mitchhentges commented May 31, 2016

KiChjang commented May 31, 2016

bors-servo commented May 31, 2016

bors-servo commented Jun 1, 2016

bors-servo commented Jun 1, 2016

KiChjang commented Jun 1, 2016

highfive commented Jun 1, 2016

mitchhentges commented Jun 1, 2016

highfive commented Jun 4, 2016

mitchhentges commented Jun 4, 2016

KiChjang commented Jun 4, 2016

mitchhentges commented Jun 4, 2016

highfive commented Jun 4, 2016

KiChjang commented Jun 4, 2016

bors-servo commented Jun 4, 2016

bors-servo commented Jun 4, 2016

bors-servo commented Jun 4, 2016

mitchhentges commented Jun 4, 2016 • edited

KiChjang commented Jun 4, 2016

bors-servo commented Jun 4, 2016

bors-servo commented Jun 4, 2016

mitchhentges commented May 26, 2016 •

edited by jdm

mitchhentges commented Jun 4, 2016 •

edited