As noted if bug 1248843 comment 2 there are two problems with DebugOnly: * DebugOnly increases the size of objects even in release builds, which some people don't realize and then (possibly unknowingly) get bitten by. * |DebugOnly<T> = ...;| evaluates |...| in all builds, which some people don't think through. This bug is aimed at addressing the former issue.

Review commit: https://reviewboard.mozilla.org/r/37723/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37723/

Review commit: https://reviewboard.mozilla.org/r/37725/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37725/

Review commit: https://reviewboard.mozilla.org/r/37727/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37727/

Review commit: https://reviewboard.mozilla.org/r/37729/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37729/

Review commit: https://reviewboard.mozilla.org/r/37731/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37731/

Review commit: https://reviewboard.mozilla.org/r/37733/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37733/

Review commit: https://reviewboard.mozilla.org/r/37735/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37735/

Comment on attachment 8725975 [details] MozReview Request: Bug 1253094, part 1 - Stop using DebugOnly for class/struct members in dom/. r=baku Review commit: https://reviewboard.mozilla.org/r/37723/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37723/

Comment on attachment 8725976 [details] MozReview Request: Bug 1253094, part 2 - Stop using DebugOnly for class/struct members in gfx/. r=Bas Review commit: https://reviewboard.mozilla.org/r/37725/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37725/

Comment on attachment 8725977 [details] MozReview Request: Bug 1253094, part 3 - Stop using DebugOnly for class/struct members in ipc/. r=billm Review commit: https://reviewboard.mozilla.org/r/37727/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37727/

Comment on attachment 8725978 [details] MozReview Request: Bug 1253094, part 4 - Stop using DebugOnly for class/struct members in js/. r=billm Review commit: https://reviewboard.mozilla.org/r/37729/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37729/

Comment on attachment 8725979 [details] MozReview Request: Bug 1253094, part 5 - Stop using DebugOnly for class/struct members in layout/. r=mats Review commit: https://reviewboard.mozilla.org/r/37731/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37731/

Comment on attachment 8725980 [details] MozReview Request: Bug 1253094, part 6 - Stop using DebugOnly for class/struct members in netwerk/. r=mayhemer Review commit: https://reviewboard.mozilla.org/r/37733/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37733/

Comment on attachment 8725981 [details] MozReview Request: Bug 1253094, part 7 - Stop using DebugOnly for class/struct members in storage/. r=mak Review commit: https://reviewboard.mozilla.org/r/37735/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37735/

Comment on attachment 8725975 [details] MozReview Request: Bug 1253094, part 1 - Stop using DebugOnly for class/struct members in dom/. r=baku Review commit: https://reviewboard.mozilla.org/r/37723/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37723/

Comment on attachment 8725976 [details] MozReview Request: Bug 1253094, part 2 - Stop using DebugOnly for class/struct members in gfx/. r=Bas Review commit: https://reviewboard.mozilla.org/r/37725/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37725/

Comment on attachment 8725977 [details] MozReview Request: Bug 1253094, part 3 - Stop using DebugOnly for class/struct members in ipc/. r=billm Review commit: https://reviewboard.mozilla.org/r/37727/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37727/

Comment on attachment 8725978 [details] MozReview Request: Bug 1253094, part 4 - Stop using DebugOnly for class/struct members in js/. r=billm Review commit: https://reviewboard.mozilla.org/r/37729/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37729/

Comment on attachment 8725979 [details] MozReview Request: Bug 1253094, part 5 - Stop using DebugOnly for class/struct members in layout/. r=mats Review commit: https://reviewboard.mozilla.org/r/37731/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37731/

Comment on attachment 8725980 [details] MozReview Request: Bug 1253094, part 6 - Stop using DebugOnly for class/struct members in netwerk/. r=mayhemer Review commit: https://reviewboard.mozilla.org/r/37733/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37733/

Comment on attachment 8725981 [details] MozReview Request: Bug 1253094, part 7 - Stop using DebugOnly for class/struct members in storage/. r=mak Review commit: https://reviewboard.mozilla.org/r/37735/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37735/

Review commit: https://reviewboard.mozilla.org/r/37737/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37737/

Review commit: https://reviewboard.mozilla.org/r/37739/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37739/

Review commit: https://reviewboard.mozilla.org/r/37741/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37741/

Review commit: https://reviewboard.mozilla.org/r/37743/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37743/

Comment on attachment 8725998 [details] MozReview Request: Bug 1253094, part 8 - Stop using DebugOnly for class/struct members in uriloader/. r=smaug https://reviewboard.mozilla.org/r/37737/#review34279 r=me

Comment on attachment 8725979 [details] MozReview Request: Bug 1253094, part 5 - Stop using DebugOnly for class/struct members in layout/. r=mats https://reviewboard.mozilla.org/r/37731/#review34281

Comment on attachment 8725999 [details] MozReview Request: Bug 1253094, part 9 - Stop using DebugOnly for class/struct members in xpcom/. r=froydnj https://reviewboard.mozilla.org/r/37739/#review34289

Comment on attachment 8726000 [details] MozReview Request: Bug 1253094, part 10 - Stop using DebugOnly for class/struct members in memory/. r=njn https://reviewboard.mozilla.org/r/37741/#review34331

Comment on attachment 8725975 [details] MozReview Request: Bug 1253094, part 1 - Stop using DebugOnly for class/struct members in dom/. r=baku https://reviewboard.mozilla.org/r/37723/#review34347

Comment on attachment 8725981 [details] MozReview Request: Bug 1253094, part 7 - Stop using DebugOnly for class/struct members in storage/. r=mak https://reviewboard.mozilla.org/r/37735/#review34359 I think we will actually expose this field in any build shortly, but for now it's fine to ifdef it.

Comment on attachment 8725980 [details] MozReview Request: Bug 1253094, part 6 - Stop using DebugOnly for class/struct members in netwerk/. r=mayhemer https://reviewboard.mozilla.org/r/37733/#review34403

Comment on attachment 8725978 [details] MozReview Request: Bug 1253094, part 4 - Stop using DebugOnly for class/struct members in js/. r=billm https://reviewboard.mozilla.org/r/37729/#review34519

Comment on attachment 8725977 [details] MozReview Request: Bug 1253094, part 3 - Stop using DebugOnly for class/struct members in ipc/. r=billm https://reviewboard.mozilla.org/r/37727/#review34521

Comment on attachment 8726001 [details] MozReview Request: Bug 1253094, part 11 - Make DebugOnly a MOZ_STACK_ONLY class. r=Waldo https://reviewboard.mozilla.org/r/37743/#review34573

I'd like to understand this better... (In reply to Jonathan Watt [:jwatt] from comment #0) > As noted if bug 1248843 comment 2 there are two problems with DebugOnly: > > * DebugOnly increases the size of objects even in release builds, which some > people don't realize and then (possibly unknowingly) get bitten by. Why? And if this is true, can't we just fix it? > * |DebugOnly<T> = ...;| evaluates |...| in all builds, which some people > don't > think through. This is exactly why I like it... for checking results of functions that need to be executed in release builds but only checked in debug. Maybe this was discussed in some thread already, just curious.

(In reply to Bas Schouten (:bas.schouten) from comment #36) > I'd like to understand this better... Given it was my comment originally, maybe I should take this. :-) > > * DebugOnly increases the size of objects even in release builds, which some > > people don't realize and then (possibly unknowingly) get bitten by. > > Why? And if this is true, can't we just fix it? The size of any class/struct must be at least 1 to ensure that the addresses of distinct objects of the same type are never the same. (C++ has special verbiage going out of the way to [attempt to] compress an empty class into zero space when it's a base class.) This generally requires that DebugOnly occupy a byte in opt builds. Standard ABIs *could* do special work to compress (some) empty class members into nothingness, but they do not. There are compiler-specific, ABI-altering ways to maybe evade this, like __attribute__((packed)). But they're specified on the *enclosing* class, not on DebugOnly. To the best of my knowledge, there's no attribute we could place on DebugOnly itself to permit optimizing it away entirely, that wouldn't require the user do something special. (And then why not just #ifdef DEBUG for absolute clarity?) > > * |DebugOnly<T> = ...;| evaluates |...| in all builds, which some people > > don't think through. > > This is exactly why I like it... for checking results of functions that need > to be executed in release builds but only checked in debug. Sure, if you think it through that way. The problem is there are people who don't think through that these DebugOnly<T> t1 = ...; t2 = ...; will *always* evaluate the ... when |t1| and |t2| are DebugOnly. They see that they're assigning to a DebugOnly, and they incorrectly jump to thinking that means the right-hand-side isn't evaluated in opt builds. That's nonsense: DebugOnly doesn't affect normal C++ evaluation semantics. But if you're thinking sloppily, it's not a surprising leap to make. In contrast, it seems much more likely developers understand the preprocessing concept enough to realize that #ifdef DEBUG T t1 = ...; t2 = ...; t3 = #endif ...; *only* evaluates the first two ... in debug builds and *always* evaluates the third ... regardless of debug/non-debug status.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #37) > > > * |DebugOnly<T> = ...;| evaluates |...| in all builds, which some people > > > don't think through. > > > > This is exactly why I like it... for checking results of functions that need > > to be executed in release builds but only checked in debug. > > Sure, if you think it through that way. The problem is there are people who > don't think through that these Anyway, for the purposes of the review here please don't get distracted by that point. :) This bug only takes action against the former (size) issue. I'd also personally still lean in favor of keeping DebugOnly for use on the stack, which is one reason this bug only focuses on the heap allocated cases. I just mentioned it for completeness. Anyway, let's lobby for that in any bug filed to kill off DebugOnly...if someone actually wants to spend time writing those patches.

Comment on attachment 8725976 [details] MozReview Request: Bug 1253094, part 2 - Stop using DebugOnly for class/struct members in gfx/. r=Bas https://reviewboard.mozilla.org/r/37725/#review34993

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dcd795e8823 https://hg.mozilla.org/integration/mozilla-inbound/rev/1bfaa66eb327 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d0c889a1c0e https://hg.mozilla.org/integration/mozilla-inbound/rev/cde0572b59f2 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d29ea20c231 https://hg.mozilla.org/integration/mozilla-inbound/rev/3365467dab5b https://hg.mozilla.org/integration/mozilla-inbound/rev/216e73b3ea18 https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ff360d4abf https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd442fbf419 https://hg.mozilla.org/integration/mozilla-inbound/rev/2e601ce0f78e

https://hg.mozilla.org/mozilla-central/rev/5dcd795e8823 https://hg.mozilla.org/mozilla-central/rev/1bfaa66eb327 https://hg.mozilla.org/mozilla-central/rev/6d0c889a1c0e https://hg.mozilla.org/mozilla-central/rev/cde0572b59f2 https://hg.mozilla.org/mozilla-central/rev/4d29ea20c231 https://hg.mozilla.org/mozilla-central/rev/3365467dab5b https://hg.mozilla.org/mozilla-central/rev/216e73b3ea18 https://hg.mozilla.org/mozilla-central/rev/b1ff360d4abf https://hg.mozilla.org/mozilla-central/rev/3dd442fbf419 https://hg.mozilla.org/mozilla-central/rev/2e601ce0f78e https://hg.mozilla.org/mozilla-central/rev/58475d84315a